Closed
Bug 467442
Opened 16 years ago
Closed 10 years ago
Autocomplete popup should take into account for -moz-transform
Categories
(Core :: XUL, defect)
Core
XUL
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Updated•16 years ago
|
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
Comment 1•13 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
I can't believe this has been known for more than 4 years now and still hasn't been fixed.
Comment 9•11 years ago
|
||
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•11 years ago
|
||
+1 for this. Is there even a work around?
Comment 11•11 years ago
|
||
+1 This is an increasingly common scenario as people are using transforms more and more.
Updated•11 years ago
|
Component: Autocomplete → Layout: Form Controls
Product: Toolkit → Core
Updated•11 years ago
|
Summary: Autocomplete should take into account for -moz-transform perhaps → Autocomplete popup should take into account for -moz-transform
Comment 13•11 years ago
|
||
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•10 years ago
|
||
Just confirmed in Nightly due to user complaint about this bug.
Comment 16•10 years ago
|
||
Here is a test-case in JS Fiddle. This bug is really annoying.
http://jsfiddle.net/MightyPork/T8M7g/1/
Assignee | ||
Comment 17•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Component: Layout: Form Controls → XP Toolkit/Widgets: Menus
Comment 18•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8436155 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Try push: remote: https://tbpl.mozilla.org/?tree=Try&rev=22c61303b34d
Assignee | ||
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
Right attachment this time...
Attachment #8437055 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8436879 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8437053 -
Attachment is obsolete: true
Attachment #8437053 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 24•10 years ago
|
||
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 25•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8436879 -
Attachment is obsolete: false
Assignee | ||
Updated•10 years ago
|
Attachment #8437055 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8436879 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
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 28•10 years ago
|
||
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 29•10 years ago
|
||
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+
Assignee | ||
Comment 30•10 years ago
|
||
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+]
Comment 31•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [fixed-in-inbound] p=3 s=33.1 [qa+] → p=3 s=33.1 [qa+]
Assignee | ||
Comment 33•10 years ago
|
||
(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...
Assignee | ||
Comment 34•10 years ago
|
||
Adjusted patch. Try push: https://tbpl.mozilla.org/?tree=Try&rev=5b25fcef51d7
Assignee | ||
Updated•10 years ago
|
Attachment #8437533 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
Green try post-retrigger-pocalypse, so relanded as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9c61ae1688
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 38•10 years ago
|
||
(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•10 years ago
|
||
Although it's still a mystery how SetPopupPosition gets called in that situation.
Comment 40•10 years ago
|
||
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!]
Comment 41•10 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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.
Updated•6 years ago
|
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in
before you can comment on or make changes to this bug.
Description
•