Closed Bug 1176519 Opened 9 years ago Closed 9 years ago

Need to press Ctrl+L twice to bring up the location bar in fullscreen mode

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox39 --- unaffected
firefox40 + fixed
firefox41 + verified

People

(Reporter: dao, Assigned: xidorn)

References

Details

(Keywords: regression, Whiteboard: [Bugday-20150701])

Attachments

(2 files)

I believe this is a recent regression.
Flags: needinfo?(quanxunzhen)
[Tracking Requested - why for this release]:
I was able to repro this on Firefox 40.0a2 (2015-06-23) build. Adding a tracking flag.
Attached patch patchSplinter Review
Assignee: nobody → quanxunzhen
Flags: needinfo?(quanxunzhen)
Attachment #8625577 - Flags: review?(dao)
This patch looks like a workaround for a deeper issue. What change exactly caused this bug?
Flags: needinfo?(quanxunzhen)
I won't see this as a workaround. We might eventually solve all of these things via CSS (with :focus-within, which we don't currently support), but changing the status first shouldn't be a bad idea anyway.

The inner reason is that, mouse tracker check the position immediately, and if that check succeeds, it calls hideNavToolbar directly. So this bug might be due to the change of where the collapsed flag is set and checked in {show,hide}NavToolbar.
Flags: needinfo?(quanxunzhen)
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> I won't see this as a workaround. We might eventually solve all of these
> things via CSS (with :focus-within, which we don't currently support),

Let's worry about that when we actually want to do that.

> The inner reason is that, mouse tracker check the position immediately, and
> if that check succeeds, it calls hideNavToolbar directly. So this bug might
> be due to the change of where the collapsed flag is set and checked in
> {show,hide}NavToolbar.

Can we fix that?
(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> > I won't see this as a workaround. We might eventually solve all of these
> > things via CSS (with :focus-within, which we don't currently support),
> 
> Let's worry about that when we actually want to do that.

Not now anyway. I've filed bug 1176997 for :focus-within but I don't think it would be fixed soonish.

> > The inner reason is that, mouse tracker check the position immediately, and
> > if that check succeeds, it calls hideNavToolbar directly. So this bug might
> > be due to the change of where the collapsed flag is set and checked in
> > {show,hide}NavToolbar.
> 
> Can we fix that?

I would rather see moving flag setting around as a workaround. But I guess we can do that. Not sure whether that would introduce new regression though.

Alternatively, we can make mouse tracker call the callback asynchronously, but I'm not sure how dangerous it would be.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8)
> > > The inner reason is that, mouse tracker check the position immediately, and
> > > if that check succeeds, it calls hideNavToolbar directly. So this bug might
> > > be due to the change of where the collapsed flag is set and checked in
> > > {show,hide}NavToolbar.
> > 
> > Can we fix that?
> 
> I would rather see moving flag setting around as a workaround. But I guess
> we can do that. Not sure whether that would introduce new regression though.

We should restore pre-bug 947854 behavior here. That shouldn't introduce new regressions.
Attached patch another patchSplinter Review
(In reply to Dão Gottwald [:dao] from comment #9)
> We should restore pre-bug 947854 behavior here. That shouldn't introduce new
> regressions.

That could... but not sure anyway. Let's see.
Attachment #8625808 - Flags: review?(dao)
Comment on attachment 8625808 [details] [diff] [review]
another patch

>     // Track whether mouse is near the toolbox
>-    this._isChromeCollapsed = false;
>     if (trackMouse && !this.useLionFullScreen) {
>       let rect = gBrowser.mPanelContainer.getBoundingClientRect();
>       this._mouseTargetRect = {
>         top: rect.top + 50,
>         bottom: rect.bottom,
>         left: rect.left,
>         right: rect.right
>       };
>       MousePosTracker.addListener(this);
>     }
>+    this._isChromeCollapsed = false;

nit: please add a blank line before this._isChromeCollapsed = false;
Attachment #8625808 - Flags: review?(dao) → review+
Attachment #8625577 - Flags: review?(dao)
https://hg.mozilla.org/mozilla-central/rev/f021a650dd49
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment on attachment 8625808 [details] [diff] [review]
another patch

Approval Request Comment
[Feature/regressing bug #]: bug 947854
[User impact if declined]: As the summary and comment 1 say
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: not clear
[String/UUID change made/needed]: n/a
Attachment #8625808 - Flags: approval-mozilla-aurora?
Comment on attachment 8625808 [details] [diff] [review]
another patch

Recent regression, taking it.
Attachment #8625808 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I have successfully reproduced the bug in Nightly 41.0a1 (2015-06-20) and windows 7 (32 bit) 
(Build ID: 20150620030209) 

Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0

Verified as fixed with latest aurora 41.0a2 (2015-06-29)
(Build ID: 20150629134017)

Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0
Successfully reproduce this bug on Nightly  41.0a1 (2015-06-20) (Build ID: 20150620030209) and Linux x64 

Bug is now fixed on latest Aurora 41.0a2 (2015-06-30)

Build ID: 20150630004006
Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [Bugday-20150701]
Whiteboard: [Bugday-20150701]
You need to log in before you can comment on or make changes to this bug.