Closed
Bug 961431
Opened 11 years ago
Closed 11 years ago
Autocomplete suggestions in split console are sliding down
Categories
(DevTools :: Console, defect, P2)
Tracking
(firefox28 unaffected, firefox29 affected, firefox30 verified, firefox31 verified)
VERIFIED
FIXED
Firefox 31
Tracking | Status | |
---|---|---|
firefox28 | --- | unaffected |
firefox29 | --- | affected |
firefox30 | --- | verified |
firefox31 | --- | verified |
People
(Reporter: merihakar, Assigned: tnikkel)
References
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
enndeakin
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
While typing into input are of split console, the suggestion box starts to move down after second letter and eventually disappears from screen.
Comment 1•11 years ago
|
||
I can reproduce this, here's a screencast:
http://youtu.be/EfWFLIoysNc
As in the video, to reproduce try opening the toolbox and then the split console, then immediately type in 'doc', hit esc to complete, then type 'l'. I can't get it to happen every single time, but it does happen most often when the toolbox / split console have just been opened. Doing the same actions a second time gets me the correct behaviour, and I need to close and then re-open the toolbox to see the bug again.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 2•11 years ago
|
||
The issue can also be reproduced as following:
1) open developer tools
2) select any tab other than console
3) open split console view
4) type 'g' (popup is not sliding down)
5) type 'e' and the suggestion box starts to slide down
By the way this is working properly in Mac OS X.
Comment 3•11 years ago
|
||
This is a bad bug, but I cannot reproduce it.
Girish, can you reproduce the bug?
Flags: needinfo?(scrapmachines)
Comment 4•11 years ago
|
||
I used to be able to reproduce it at random times on my (a bit) slower windows machine. Will try to figure out if I can find an obvious root cause.
But I strongly think that this is a platform issue as nothing from the popup code changed recently.
Flags: needinfo?(scrapmachines)
Comment 5•11 years ago
|
||
Attaching a potential fix. This patch removes all workarounds for popup dynamic width, and now we use a nicer alternative with css and js.
Try push: https://tbpl.mozilla.org/?tree=Try&rev=f576ad4e8391
Builds will be available here soonish: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-f576ad4e8391/
Jeff, can you please test one of these builds?
Flags: needinfo?(jgriffiths)
Comment 6•11 years ago
|
||
:msucan - tried the MacOSX64 build, in my testing I was able to easily reproduce the problem on current nightly, but could not reproduce on your try build using the same test sequence. Looks fixed to me!
Flags: needinfo?(jgriffiths)
Comment 7•11 years ago
|
||
(In reply to Jeff Griffiths (:canuckistani) from comment #6)
> :msucan - tried the MacOSX64 build, in my testing I was able to easily
> reproduce the problem on current nightly, but could not reproduce on your
> try build using the same test sequence. Looks fixed to me!
Thank you for testing! That sounds great. Did you see any jumpy behavior at all or other weird things? I will finish up the patch next week.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Comment 8•11 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #7)
...
> Thank you for testing! That sounds great. Did you see any jumpy behavior at
> all or other weird things? I will finish up the patch next week.
It looks pretty normal to me. Certainly both much better than current nightly, and not surprising.
Comment 9•11 years ago
|
||
Mihai, while we are at removing hacks and calculations, I suggest adding the width calculation and assignment part at all.
In rules view, we use the fixed width mode, with no (min-)width specified to the popup. Just a max-width, this makes the popup take up as much size it wants horizontally. We can do that here with web console popup too.
Wanna give it a shot ?
Comment 10•11 years ago
|
||
(In reply to Girish Sharma [:Optimizer] from comment #9)
> Mihai, while we are at removing hacks and calculations, I suggest adding the
> width calculation and assignment part at all.
> In rules view, we use the fixed width mode, with no (min-)width specified to
> the popup. Just a max-width, this makes the popup take up as much size it
> wants horizontally. We can do that here with web console popup too.
>
> Wanna give it a shot ?
I just tried what you suggested and for some reason in the webconsole when I type fast I can see the popup showing with the wrong width, then it shows scrollbars when it shouldnt. No min-width, and I also tried the hidePopup/openPopup() cycle on every update. No luck.
I'm going to keep the width calculation - it's simple enough and it's not a hack. In fact, I am making it the only option - I am removing the "fixedWidth" option (which is a misnomer).
Comment 11•11 years ago
|
||
Ready for review.
New try push: https://tbpl.mozilla.org/?tree=Try&rev=54e212c18b4b
New builds to test: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-54e212c18b4b/
Attachment #8365236 -
Attachment is obsolete: true
Attachment #8366620 -
Flags: review?(rcampbell)
Comment 12•11 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #10)
> (In reply to Girish Sharma [:Optimizer] from comment #9)
> > Mihai, while we are at removing hacks and calculations, I suggest adding the
> > width calculation and assignment part at all.
> > In rules view, we use the fixed width mode, with no (min-)width specified to
> > the popup. Just a max-width, this makes the popup take up as much size it
> > wants horizontally. We can do that here with web console popup too.
> >
> > Wanna give it a shot ?
>
> I just tried what you suggested and for some reason in the webconsole when I
> type fast I can see the popup showing with the wrong width, then it shows
> scrollbars when it shouldnt. No min-width, and I also tried the
> hidePopup/openPopup() cycle on every update. No luck.
Did you close and open it again on each request ? Unless you close it and then open it again, things will not be okay.
> I'm going to keep the width calculation - it's simple enough and it's not a
> hack. In fact, I am making it the only option - I am removing the
> "fixedWidth" option (which is a misnomer).
I am okay with this, but who knows tomorrow some other XUL Panel related issue will regress this hack too.
Updated•11 years ago
|
Attachment #8366620 -
Flags: review?(rcampbell) → review+
Comment 14•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 15•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Reporter | ||
Comment 16•11 years ago
|
||
Suggestion box is still sliding down in Nightly 30.0a1 (2014-02-05):
After I wrote "document." to split console, when I type "g" the suggestion box starts to slide down. If I delete "g", the box appears way above the input area.
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•11 years ago
|
||
Can we backout this patch to at least not cause a "floating up" regression ?
Comment 18•11 years ago
|
||
Jeff, can you reproduce the sliding behavior on latest Nightly too ?
If you can still, lets backout this patch so as to get rid of atleast the regression caused by this patch.
Flags: needinfo?(jgriffiths)
Comment 19•11 years ago
|
||
Current nightly works great for me ( OS X 10.9 ). I don't see a problem here.
Flags: needinfo?(jgriffiths)
Reporter | ||
Comment 20•11 years ago
|
||
(In reply to Jeff Griffiths (:canuckistani) from comment #19)
> Current nightly works great for me ( OS X 10.9 ). I don't see a problem here.
As I've mentioned in Comment #2 there is no problem in OS X.
I can check the current Nightly in Windows in a minute.
Comment 21•11 years ago
|
||
I personally was not able to reproduce sliding behavior even previously on windows, so I can't really tell if it is still there or not. What I can tell is that the popup's vertical alignment issue that was existing in the past but was fixed has reappeared due to this patch.
Comment 22•11 years ago
|
||
I'd rather log a follow-up for fixing the regression and have non-sliding pop-ups. Please don't yank this patch, instead fix the alignment in a followup.
Comment 23•11 years ago
|
||
comment 16 says they are still sliding down .
Reporter | ||
Comment 24•11 years ago
|
||
Yes the popup is sliding down in windows ff beta (28.0), ff aurora 29.0a2 (2014-02-25) and ff nightly 30.0a1 (2014-02-25).
I could not reproduce it on the same machine with Ubuntu nightly 29.0a1 (2014-01-08).
It may have something to do with my configuration, so I can provide anything that can help.
Comment 26•11 years ago
|
||
I'm the author of a duplicate bug report, i'd like to add here that in the latest nightly in OSX the bug is fixed.
Reporter | ||
Comment 27•11 years ago
|
||
(In reply to guillermo siliceo from comment #26)
> I'm the author of a duplicate bug report, i'd like to add here that in the
> latest nightly in OSX the bug is fixed.
For me, OS X was OK from the beginning, and sliding behavior happens only in split console mode. As I looked at your report, your suggestion box flies in console tab.
Now that I checked again with latest updates, the problem persists in beta, aurora and nightly. Also in nightly suggestion box appears slightly above the input area in console tab.
Comment 28•11 years ago
|
||
I'm assigned to this bug and I see there was discussion about what to do here (backout, open follow-up, etc).
- it looks like XUL panels are broken beyond recognition. We are trying to fix here the 'unfixable'. Instead we should get someone from platform to fix the bugs.
- this patch removes ugly workarounds which fixes issues for some of our users, as evidenced by the duplicate report(s) and testing done within our team (me, jeff and perhaps others).
- to backout this patch means to trade between issues. It's not 'fixing' stuff. The only merit I see for keeping this patch in is it's less mess in JS.
- going forward we either get a platform fix, or we just replace our use of the XUL popup with plain HTML that actually works well. Continuing to write patches with workarounds for XUL bugs is counterproductive.
- we cant mark this bug as fixed, either. The popup is still (seriously) broken.
I am unassigning myself from this bug - I dont know yet when I will have time to rewrite the popup with HTML. Girish, do you want to help with this?
Updated•11 years ago
|
Assignee: mihai.sucan → nobody
Component: Developer Tools → Developer Tools: Console
Comment 29•11 years ago
|
||
Even I don't have that much time for a complete rework of the popup in HTML.
I think at least the flying popup issues can be simply solved by adding popup.hidePopup();popup.openPopup() calls on each keypress.
The sliding problem, as I suggested previously too, is an XUL issue which is not in our hand.
Let's at least not worsen the experience for Windows user (which is arguably the major population of console users)
Comment 30•11 years ago
|
||
(volunteering to mentor someone who wants to help with migrating the autocomplete popup from XUL to HTML)
Whiteboard: [mentor=msucan][lang=js,css,html]
Comment 31•11 years ago
|
||
Bug still here in Firefox 28.
Comment 32•11 years ago
|
||
Is there any way to entirely disable this feature? If not configurable, maybe a compile flag?
Comment 33•11 years ago
|
||
(In reply to guillermo siliceo from comment #32)
> Is there any way to entirely disable this feature? If not configurable,
> maybe a compile flag?
There's no such option.
Priority: -- → P2
Comment 34•11 years ago
|
||
This is another one of these rounding issues caused by fractional display pixels. I can easily reproduce on Windows but not on other platforms.
In my case rootFrame->GetScreenRectInAppUnits() is returning a y value of 54510 (908.5 pixels). The viewPoint.y is computed in SetPopupPosition to a value of 1530 (25.5 pixels). This would mean that the desired position of the popup is both values added together which is 934 pixels. This gets rounded up to 1560 (26 pixels) here http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuPopupFrame.cpp#1342 which is the code added by bug 622507.
Then, later when CalcWidgetBounds is called the 1560 value is added to the root rect's y position of 54510 giving 56070 (934.5). This causes the line:
nsIntRect newBounds = viewBounds.ToNearestPixels(p2a);
to round this value up again, to 935 pixels, one pixel higher than was actually desired. This then causes a loop and the popup drifts downwards.
Removing the lines added by bug 622507 fixes this problem. Maybe Timothy can shed some light as to whether this change should be reverted or there is some other issue.
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 35•11 years ago
|
||
Thanks for that analysis! Made it a lot easier.
After reading through bug 622507 I think a mistake was made in implementing their chosen fix. I think screenPosition is what should be rounded in nsMenuPopupFrame::SetPopupPosition (the position in screen coords of the popup). viewPoint is the position of the popup relative to it's parent view (the root view). But the widget of the popup's view has it's coordinates in screen coordinates. So I can see why it would be an easy mistake to make (the setup is confusing), and it would even fix the bug as long as the root view didn't have fractional coordinates (they don't usually, or didn't until we got fractional display pixels). But in general we shouldn't round the offset between two views to pixels.
Blocks: 622507
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 36•11 years ago
|
||
Attachment #8398387 -
Flags: review?(enndeakin)
Comment 37•11 years ago
|
||
Comment on attachment 8398387 [details] [diff] [review]
round screen point of popups
Review of attachment 8398387 [details] [diff] [review]:
-----------------------------------------------------------------
That makes sense to me.
Attachment #8398387 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 38•11 years ago
|
||
Assignee | ||
Comment 39•11 years ago
|
||
Comment 40•11 years ago
|
||
Assignee: nobody → tnikkel
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 41•11 years ago
|
||
Thank you Neil and Timothy!
Is it safe to get this fix in aurora as well? If yes, can you please do this?
Flags: needinfo?(tnikkel)
Whiteboard: [mentor=msucan][lang=js,css,html]
Comment 42•11 years ago
|
||
Thank you!
Assignee | ||
Comment 43•11 years ago
|
||
Comment on attachment 8398387 [details] [diff] [review]
round screen point of popups
[Approval Request Comment]
Bug caused by (feature/regressing bug #): the fix for bug 622507 was only partial. it only handled non-hidpi cases properly
User impact if declined: popup panels move when they shouldn't
Testing completed (on m-c, etc.): on m-c for several days now
Risk to taking this patch (and alternatives if risky): pretty low risk as far as panel/popup fixes go
String or IDL/UUID changes made by this patch: none
Attachment #8398387 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 44•11 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #42)
> Thank you!
Sorry that you have to deal with buggy panels!
Comment 45•11 years ago
|
||
Thanks for the proper fix guys :)
While we are at it, I also filed Bug 990631. It would be really nice if we do not have to add back the hacks we removed in the first landing of this bug (which caused the issue).
That issue is also a core XUL issue. I'd really appreciate if there can be a fix from the XUL side, rather then us adding hacks to avoid the issue.
Thanks.
Updated•11 years ago
|
Attachment #8398387 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
status-firefox28:
--- → unaffected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → verified
Assignee | ||
Comment 46•11 years ago
|
||
Updated•11 years ago
|
Target Milestone: Firefox 29 → Firefox 31
Updated•11 years ago
|
QA Whiteboard: [good first verify]
Comment 47•10 years ago
|
||
Hi,
I wasn't able to reproduce it on Linux (Debian Sid, x86_64) and as no one was able to in the comments, I'm assuming Linux isn't affected.
I was able to reproduce it on Windows 7 x86_64 with Nightly 29.0a1 2014-01-19 and I can confirm it's fixed in latest Aurora (31.0a2 2014-05-29) and Beta (30.0 2014-05-27).
I'm not changing the status flags to verified as I don't have a Mac machine to test the fix on: if despite the All/All field, it's ok for you only to have it verified on Windows, feel free to change the status flags accordingly.
Cheers,
Francesca
QA Whiteboard: [good first verify] → [good first verify] [testday-20140530]
Updated•10 years ago
|
Whiteboard: 979987
Updated•10 years ago
|
Whiteboard: 979987
Comment 48•10 years ago
|
||
As mentioned in comment 2 and comment 20, the issue does not affect Mac OS X (I've also tried to reproduce it on a Mac OS X 10.9.2, with Nightly 29.0a1 2014-01-19, but with no success... note that it easily reproduced on Windows 7).
As a consequence, based on Francesca's testing, I'm marking this as Verified for both 31 Aurora and 30 Beta.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•