467442 - Autocomplete popup should take into account for -moz-transform

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, if you have some autocomplete items stored with Google, then you should see also an autocomplete popup appearing when double-clicking on one of the text inputs.

The autocomplete popups are seemingly appearing on illogical positions, that happens because the autocomplete doesn't take into account for elements (or their ancestors) that are transformed with -moz-transform.

Perhaps, the code should at least make sure that no autocomplete popup can appear at the place where is mousedowned.

Comment 1

[Andrew Udvare]

What do you mean perhaps? Of COURSE it should take it into account.

Every other browser does (although I cannot say for IE 10). This is affecting a site I'm working on right now.

My solution is to disable autocomplete for Firefox for that site because this is completely unintuitive and embarrassing.

This is probably related to the fact that Firefox STILL does not take into account -moz-transform when doing calculations of offsets with offsetLeft, offsetRight and window.getComputedStyle (which again is unlike every other browser including IE 10).

Comment 5

[Matt Basta [:basta]]

This issue also happens in Webkit under some circumstances.

As bug 823289 mentions, for those interested, if the input isn't translated but its ancestors are, setting transform:none on the input or parent seems to help.

Comment 6

[Luc Heinrich]

I can't believe this has been known for more than 4 years now and still hasn't been fixed.

Comment 9

[Ben Saufley]

I do not develop browsers so I don't know how complicated fixing this would be. But I would certainly love to know why it hasn't been – a reason why, as Luc Heinrich points out, this bug was added 5 1/2 years ago and is not even assigned? This doesn't seem trivial - *of course* transforms should be accounted for when positioning an autocomplete window? What am I missing? A lot of duplicate bugs have been added and subsequently marked as duplicate, but there's still no activity on the original?

Comment 10

[Andrew Bullock]

+1 for this. Is there even a work around?

Comment 11

[Pedro Valentim]

+1 This is an increasingly common scenario as people are using transforms more and more.

Comment 13

[Harlan Lewis]

Comment on attachment 350865 [details]
testcase

I had some difficulty making autosuggest values appear at all in the above test.

I've created an additional sample, displaying the bug in a common use case that's easy to reproduce. http://codepen.io/anon/pen/Luymn/

Comment 15

[Tyler Downer [He/Him]]

Just confirmed in Nightly due to user complaint about this bug.

Comment 16

[Ondřej Hruška]

Here is a test-case in JS Fiddle. This bug is really annoying.

http://jsfiddle.net/MightyPork/T8M7g/1/

Comment 17

[:Gijs (he/him)]

Attachment: take transforms into account for popup placement,

This seems to work for the last few testcases added here. I could never get an autocomplete popup to show with the original testcase. Asking for feedback only because I guess there should be a test for this - probably a mochitest-chrome?

Comment 18

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8436155 [details] [diff] [review]
take transforms into account for popup placement,

Looks good.

This might get weird results for transform other than shifts and scales, but I'm not sure if it'd be any worse then what we do currently.

Comment 19

[:Gijs (he/him)]

Attachment: take transforms into account for popup placement,

And this time with a generic chrome test for popup placement (as that seemed a lot simpler than trying to write a functional autocomplete test, and really, autocomplete should be able to trust this bit of layout code).

Comment 20

[:Gijs (he/him)]

Try push: remote:   https://tbpl.mozilla.org/?tree=Try&rev=22c61303b34d

Comment 21

[:Gijs (he/him)]

Comment on attachment 8436879 [details] [diff] [review]
take transforms into account for popup placement,

This makes toolkit/content/tests/chrome/test_popup_anchor.xul go orange across the board, so I should look at that first. Clearing review for now.

Comment 22

[:Gijs (he/him)]

Attachment: try: -bod -p win32,linux64,macosx64 -t none -u mochitest-o

This seems to fix it. Not sure it's the right fix, though... Try push: https://tbpl.mozilla.org/?tree=Try&rev=4ed00b5f7376

Comment 23

[:Gijs (he/him)]

Attachment: take transforms into account for popup placement,

Right attachment this time...

Comment 24

[:Gijs (he/him)]

Comment on attachment 8437055 [details] [diff] [review]
take transforms into account for popup placement,

Delightful. Fix one thing, break another... good thing we have tests, I guess:

3962 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_popup_scaled.xul | anchored left position - got 108, expected 46
3963 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_popup_scaled.xul | anchored top position - got 53, expected 0

I'm not sure what's up. This time, I'm going to leave a feedback? here and I would appreciate a pointer as to what I'm getting wrong this time...

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8437055 [details] [diff] [review]
take transforms into account for popup placement,

>+  nsIFrame* referenceFrame = rootFrame;
>   // if the frame is not specified, use the anchor node passed to OpenPopup. If
>   // that wasn't specified either, use the root frame. Note that mAnchorContent
>   // might be a different document so its presshell must be used.
>   if (!aAnchorFrame) {
>     if (mAnchorContent) {
>       aAnchorFrame = mAnchorContent->GetPrimaryFrame();
>     }
> 
>     if (!aAnchorFrame) {
>       aAnchorFrame = rootFrame;
>       if (!aAnchorFrame)
>         return NS_OK;
>     }
>+    // For reference, use the new anchor frame's root to determine transform
>+    // and screen position:
>+    referenceFrame = aAnchorFrame->PresContext()->FrameManager()->GetRootFrame();
>   }
> 
>   // the dimensions of the anchor in its app units
>-  nsRect parentRect = aAnchorFrame->GetScreenRectInAppUnits();
>-
>-  // the anchor may be in a different document with a different scale,
>-  // so adjust the size so that it is in the app units of the popup instead
>-  // of the anchor.
>-  parentRect = parentRect.ConvertAppUnitsRoundOut(
>-    aAnchorFrame->PresContext()->AppUnitsPerDevPixel(),
>-    presContext->AppUnitsPerDevPixel());
>+  nsRect parentRect = aAnchorFrame->GetRectRelativeToSelf();
>+  parentRect = nsLayoutUtils::TransformFrameRectToAncestor(aAnchorFrame, parentRect, referenceFrame);
>+  parentRect.MoveBy(referenceFrame->GetScreenRectInAppUnits().TopLeft());

To make this work we are relying on the fact that we can convert from coords relative to referenceFrame to coords relative to the screen. But the anchor element could be in any document which has any CSS transform applied, so we can't easily convert from the root frame of the document that anchor element is in to the screen.

Your first patch had this right. Did you change this to pass test_popup_anchor.xul? I don't think this is the right fix.

test_popup_anchor.xul is failing because in that test we use TransformFrameRectToAncestor where the ancestor frame we pass is not actually an ancestor of the anchor frame (because the popup frames are in a child document to the document with the anchor).

So I suggest getting the root prescontext of the prescontext of the anchor frame using GetRootPresContext, and get the root frame of that document, use that as the ancestor to transform to. And then you will need to keep the appunits conversion code that you remove here, except you are converting from prescontext of the ancestor frame you are using to the prescontext of the popup frame (presContext).

Comment 26

[:Gijs (he/him)]

Attachment: take transforms into account for popup placement,

Alright, so I think this is what you meant... but I could be wrong - let's find out: https://tbpl.mozilla.org/?tree=Try&rev=232be2a8a44b

Comment 27

[:Gijs (he/him)]

Comment on attachment 8437533 [details] [diff] [review]
take transforms into account for popup placement,

This is looking better - green m-other tests... tn, was this what you meant? :-)

Comment 28

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8437533 [details] [diff] [review]
take transforms into account for popup placement,

Yep, thats what I meant.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8437533 [details] [diff] [review]
take transforms into account for popup placement,

>+  // In order to deal with transforms, we need the root prescontext:

"with transforms, possibly on an ancestor of the <iframe> containing our anchor"?

r=me.  Thank you for putting in those comments!

Comment 30

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/21a9c0fde1db


Marco, I hope this is the last one today, but can you add this one too? Thanks!

Comment 31

[Ed Morley [:emorley]]

Backed out for crashes:
 PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_conditional-breakpoints-03.js | application crashed [@ nsMenuPopupFrame::SetPopupPosition(nsIFrame*, bool, bool)]
https://tbpl.mozilla.org/php/getParsedLog.php?id=41530970&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8dfa360a4d

Comment 32

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 33

[:Gijs (he/him)]

(In reply to Ed Morley [:edmorley UTC+0] from comment #31)
> Backed out for crashes:
>  PROCESS-CRASH |
> chrome://mochitests/content/browser/browser/devtools/debugger/test/
> browser_dbg_conditional-breakpoints-03.js | application crashed [@
> nsMenuPopupFrame::SetPopupPosition(nsIFrame*, bool, bool)]
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41530970&tree=Mozilla-
> Inbound
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8dfa360a4d

Unfortunately I've not been able to repro this locally, and there were no debug builds with this issue. Based on IRC conversations with bz and tn (thanks!), it seems like GetRootPresContext() might return null in certain (race) conditions, in which case we should bail. I'll try to write up a patch to check that case and chuck it at try with a bunch of retriggers. While the crashes were prevalent (e.g. 50% on mac 10.8, but none on mac 10.6...), they didn't always happen, nor were they always in the same test when they did happen, even per-platform-version...

Comment 34

[:Gijs (he/him)]

Attachment: take transforms into account for popup placement,

Adjusted patch. Try push: https://tbpl.mozilla.org/?tree=Try&rev=5b25fcef51d7

Comment 35

[:Gijs (he/him)]

Green try post-retrigger-pocalypse, so relanded as:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9c61ae1688

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b9c61ae1688

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to :Gijs Kruitbosch from comment #33)
> (In reply to Ed Morley [:edmorley UTC+0] from comment #31)
> > Backed out for crashes:
> >  PROCESS-CRASH |
> > chrome://mochitests/content/browser/browser/devtools/debugger/test/
> > browser_dbg_conditional-breakpoints-03.js | application crashed [@
> > nsMenuPopupFrame::SetPopupPosition(nsIFrame*, bool, bool)]
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=41530970&tree=Mozilla-
> > Inbound
> > 
> > remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8dfa360a4d
> 
> Unfortunately I've not been able to repro this locally, and there were no
> debug builds with this issue. Based on IRC conversations with bz and tn
> (thanks!), it seems like GetRootPresContext() might return null in certain
> (race) conditions, in which case we should bail. I'll try to write up a
> patch to check that case and chuck it at try with a bunch of retriggers.
> While the crashes were prevalent (e.g. 50% on mac 10.8, but none on mac
> 10.6...), they didn't always happen, nor were they always in the same test
> when they did happen, even per-platform-version...

I added some logging and pushed to try to see what was going on here. It looks like we are destroying an iframe. When we destroy an iframe we stash the presentation on the frameloader temporarily and add a script runner that will destroy it when it runs (so that if we are recreating it we can just re-use the old presentation). So the old presentation is living a little bit longer after it's view has been disconnected. Returning early seems like the correct thing to do.

Comment 39

[Timothy Nikkel (:tnikkel)]

Although it's still a mystery how SetPopupPosition gets called in that situation.

Comment 40

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 33
Build id: 20140615030204
OS: Win 7 x64, Ubuntu 12.10 x32, Mac Os X 10.8.5

Comment 41

[Timothy Nikkel (:tnikkel)]

The Windows try server had intact crash stacks. From that it looked as though the child document had just been removed, a flush was caused in it from the focus code, which was removing focus from the child document. And then there must have been something marked dirty before that causing us to reflow. The child document doesn't die immediately, we keep it alive until the next time we hit the event loop in case we are just reconstructing the subdocument frame that contains it.

Comment 42

[hi]

This bug appears like fixed, but it is still alive:

http://jsfiddle.net/opundo/cyL3qb0x

At least in Firefox 41.0.1 on Windows 10

Comment 43

[Boris Zbarsky [:bzbarsky]]

This bug is about the autocomplete popup.

The testcase in comment 42 is using a <select> popup.  For a <select> popup things work correctly for 2d translations, but the testcase is using a 3d transform.  Setting the z value there to 0, for example, makes it work correctly.  The point is, the testcase is showing bug 455164, not this bug.