Closed Bug 961431 Opened 6 years ago Closed 6 years ago

Autocomplete suggestions in split console are sliding down

Categories

(DevTools :: Console, defect, P2)

28 Branch
defect

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)

Attached image Moving suggestion box
While typing into input are of split console, the suggestion box starts to move down after second letter and eventually disappears from screen.
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
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.
This is a bad bug, but I cannot reproduce it.

Girish, can you reproduce the bug?
Flags: needinfo?(scrapmachines)
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)
Attached patch bug961431-1.diff (obsolete) — Splinter Review
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)
: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)
(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
(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.
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 ?
(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).
(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.
Attachment #8366620 - Flags: review?(rcampbell) → review+
Thank you Rob!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea49291871ef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we backout this patch to at least not cause a "floating up" regression ?
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)
Current nightly works great for me ( OS X 10.9 ). I don't see a problem here.
Flags: needinfo?(jgriffiths)
(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.
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.
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 16 says they are still sliding down .
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.
Duplicate of this bug: 976595
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.
(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.
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?
Assignee: mihai.sucan → nobody
Component: Developer Tools → Developer Tools: Console
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)
(volunteering to mentor someone who wants to help with migrating the autocomplete popup from XUL to HTML)
Whiteboard: [mentor=msucan][lang=js,css,html]
Bug still here in Firefox 28.
Is there any way to entirely disable this feature? If not configurable, maybe a compile flag?
(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
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)
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)
Attachment #8398387 - Flags: review?(enndeakin)
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+
https://hg.mozilla.org/mozilla-central/rev/21101475f295
Assignee: nobody → tnikkel
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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]
Thank you!
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)
(In reply to Rob Campbell [:rc] (:robcee) from comment #42)
> Thank you!

Sorry that you have to deal with buggy panels!
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.
Attachment #8398387 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: Firefox 29 → Firefox 31
QA Whiteboard: [good first verify]
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]
Whiteboard: 979987
Whiteboard: 979987
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.