Closed Bug 1386745 Opened 7 years ago Closed 7 years ago

Activated/focused URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063

Categories

(Firefox :: Address Bar, defect, P1)

56 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: Virtual, Assigned: adw)

References

Details

(Keywords: nightly-community, regression, reproducible, Whiteboard: [reserve-photon-animation])

Attachments

(3 files)

STR:
1. Open some website page
2. Make active URL/address/location bar
3. Press "β˜†" Bookmark this page button
and see that bookmark doorhanger/panelUI is located under "Show site information" button area
It's latest regression,
maybe caused by one of these bugs:
Bug #1374477
Bug #1385407

Any ideas or missed call?
Has STR: --- → yes
Flags: needinfo?(jaws)
Flags: needinfo?(adw)
QA Contact: Virtual
Summary: Activate URL/address/location bar makes doorhanger/panelUI located under "Show site information" button area → Activate URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area
Bug 1385407 wouldn't affect this. It just made the star hide when the user is editing the location bar.
Flags: needinfo?(jaws)
It's conceivable that bug 1374477 might have broken this, but it doesn't show up in the changelog in comment 4, and also I can't reproduce it.  And I think we have (simple) automated tests for this.
Flags: needinfo?(jaws)
Whiteboard: [photon] [triage] → [photon-animation][triage]
Summary: Activate URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area → Activate URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063
This seems to be because BookmarkingUI.anchor fails the isVisible checks here ( http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/browser/base/content/browser-places.js#483 ). TBH, I expect that while we fix this, we probably want to anchor to the page action button before using the identity block, if that's available (for instance, because the user bookmarked from the item in the page action menu but doesn't have the star visible in the url bar).

Note that we call handleRevert() right before this code runs, which will cause urlbar icons to be visible again if they were invisible before. That said, I don't really understand why they would ever stop being visible.

As a workaround, perhaps waiting for a layout flush first will help? I'd rather work out why the icon disappears at all though (it must clearly be visible in order for the user to be clicking it to bookmark the page...).
Flags: qe-verify+
Priority: -- → P4
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Summary: Activate URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063 → Activated URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063
Priority: P4 → P3
Priority: P3 → P4
Jared, I suspect this might have a similar cause to bug 1393133, where we'll set changes in motion that hide/unhide things and then try to use the anchor immediately, but, it having no size / visibility, that fails. Is that possible here? Any chance you can look into this in the next few weeks given the animation somehow regressed this? If not I can see if someone on the structure team has space to dig, but I don't know if we do.
Flags: needinfo?(jaws)
That's a good idea as to why this is happening. I'm not sure if I can take a look at it within the next few weeks. We've prioritized it as P4 which means we are OK with this not getting fixed in 57 (as you know this doesn't prevent us from fixing it in 57 though). If you or someone else has time to pick this up that would be appreciated :)
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
I can no longer reproduce this on current nightly. Tested on both OSX and Windows. Can you?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(Virtual)
Yes, I can still reproduce bug with Mozilla Firefox Nightly 57.0a1 (2017-09-07) (64-bit) [https://hg.mozilla.org/mozilla-central/rev/d8e238b811d3dc74515065ae8cab6c74baf0295f] on Windows 7 (64-bit).
Flags: needinfo?(Virtual)
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #11)
> Yes, I can still reproduce bug with Mozilla Firefox Nightly 57.0a1
> (2017-09-07) (64-bit)
> [https://hg.mozilla.org/mozilla-central/rev/
> d8e238b811d3dc74515065ae8cab6c74baf0295f] on Windows 7 (64-bit).

Can you produce a screencast of what you're doing? Seems like there must be a difference with what I'm doing somehow.
Flags: needinfo?(Virtual)
Attached video screencast.mp4 β€”
Sure thing, have a look.
Flags: needinfo?(Virtual)
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #13)
> Created attachment 8905521 [details]
> screencast.mp4
> 
> Sure thing, have a look.

I can reproduce if first bookmarking it without focus in the urlbar, and then doing it again after focus. Not sure why that makes a difference. Will try to investigate after the weekend.
Flags: needinfo?(gijskruitbosch+bugs)
The problem is that the anchor is #star-button, and #star-button is no longer visible once #star-button-animatable-image is shown on top of it.  So we fall back to the next best anchor.  I'll have a patch soon.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Iteration: --- → 57.3 - Sep 19
Priority: P4 → P1
But I'm not sure what the urlbar's being focused has to do with it (although I can reproduce the problem).
Comment on attachment 8906029 [details]
Bug 1386745 - Activated URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063.

https://reviewboard.mozilla.org/r/177786/#review182988

Awesome, thanks!
Attachment #8906029 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40ced61c2fa8
Activated URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/40ced61c2fa8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-09-10), so I'm marking this bug as VERIFIED. Thank you very much! \o/
Status: RESOLVED → VERIFIED
Keywords: reproducible
Summary: Activated URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063 → Activated/focused URL/address/location bar makes bookmark doorhanger/panelUI located under "Show site information" button area after landing patch from bug #1352063
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.