Closed Bug 467442 Opened 11 years ago Closed 6 years ago

Autocomplete popup should take into account for -moz-transform

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33

People

(Reporter: martijn.martijn, Assigned: Gijs)

References

()

Details

(Keywords: testcase, Whiteboard: p=3 s=33.1 [qa!])

Attachments

(2 files, 5 obsolete files)

Attached file 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.
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
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).
Duplicate of this bug: 777263
Duplicate of this bug: 808774
Duplicate of this bug: 823289
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.
I can't believe this has been known for more than 4 years now and still hasn't been fixed.
Duplicate of this bug: 890357
Duplicate of this bug: 932113
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?
+1 for this. Is there even a work around?
+1 This is an increasingly common scenario as people are using transforms more and more.
Duplicate of this bug: 924325
Component: Autocomplete → Layout: Form Controls
Product: Toolkit → Core
Summary: Autocomplete should take into account for -moz-transform perhaps → Autocomplete popup should take into account for -moz-transform
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/
Duplicate of this bug: 1007043
Just confirmed in Nightly due to user complaint about this bug.
Here is a test-case in JS Fiddle. This bug is really annoying.

http://jsfiddle.net/MightyPork/T8M7g/1/
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?
Attachment #8436155 - Flags: feedback?(tnikkel)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Component: Layout: Form Controls → XP Toolkit/Widgets: Menus
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.
Attachment #8436155 - Flags: feedback?(tnikkel) → feedback+
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).
Attachment #8436879 - Flags: review?(bzbarsky)
Attachment #8436155 - Attachment is obsolete: true
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.
Attachment #8436879 - Flags: review?(bzbarsky)
This seems to fix it. Not sure it's the right fix, though... Try push: https://tbpl.mozilla.org/?tree=Try&rev=4ed00b5f7376
Attachment #8437053 - Flags: review?(bzbarsky)
Right attachment this time...
Attachment #8437055 - Flags: review?(bzbarsky)
Attachment #8436879 - Attachment is obsolete: true
Attachment #8437053 - Attachment is obsolete: true
Attachment #8437053 - Flags: review?(bzbarsky)
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...
Attachment #8437055 - Flags: review?(bzbarsky) → feedback?(bzbarsky)
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).
Attachment #8437055 - Flags: feedback?(bzbarsky)
Attachment #8436879 - Attachment is obsolete: false
Attachment #8437055 - Attachment is obsolete: true
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
Attachment #8436879 - Attachment is obsolete: true
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? :-)
Attachment #8437533 - Flags: review?(bzbarsky)
Attachment #8437533 - Flags: feedback?(tnikkel)
Comment on attachment 8437533 [details] [diff] [review]
take transforms into account for popup placement,

Yep, thats what I meant.
Attachment #8437533 - Flags: feedback?(tnikkel) → feedback+
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!
Attachment #8437533 - Flags: review?(bzbarsky) → review+
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!
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Whiteboard: [fixed-in-inbound] p=3 s=33.1 [qa+]
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
Whiteboard: [fixed-in-inbound] p=3 s=33.1 [qa+] → p=3 s=33.1 [qa+]
Added to Iteration 33.1
Flags: needinfo?(mmucci)
(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...
Attachment #8437533 - Attachment is obsolete: true
Green try post-retrigger-pocalypse, so relanded as:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9c61ae1688
Duplicate of this bug: 1024049
https://hg.mozilla.org/mozilla-central/rev/1b9c61ae1688
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(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.
Although it's still a mystery how SetPopupPosition gets called in that situation.
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
Status: RESOLVED → VERIFIED
Whiteboard: p=3 s=33.1 [qa+] → p=3 s=33.1 [qa!]
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.
Depends on: 1143974
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
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.
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.