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

VERIFIED FIXED in Firefox 57

Status

()

P1
major
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Virtual, Assigned: adw)

Tracking

({nightly-community, regression, reproducible})

56 Branch
Firefox 57
x86_64
Windows 7
nightly-community, regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 verified)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(3 attachments)

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)
(Assignee)

Comment 5

2 years ago
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.
Thanks for replies.

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=30b65b87bd9a7fb1b4e4f36dbe45e872f2f6f2e2&tochange=767224f031ac7ad09261957ebbbaa2d8f178a54c
Blocks: 1352063
status-firefox56: affected → disabled
status-firefox57: --- → affected
Flags: needinfo?(jaws)
Keywords: regressionwindow-wanted
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

Comment 7

2 years ago
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

Updated

2 years ago
status-firefox57: affected → fix-optional
Priority: P4 → P3
Priority: P3 → P4

Comment 8

2 years ago
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)

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 10

2 years ago
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)

Comment 12

2 years ago
(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)
Posted video screencast.mp4
Sure thing, have a look.
Flags: needinfo?(Virtual)

Comment 14

2 years ago
(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)
(Assignee)

Comment 15

2 years ago
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
(Assignee)

Comment 17

2 years ago
But I'm not sure what the urlbar's being focused has to do with it (although I can reproduce the problem).

Comment 19

2 years ago
mozreview-review
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+

Comment 20

2 years ago
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
Last Resolved: 2 years ago
status-firefox57: fix-optional → fixed
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
status-firefox57: fixed → 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

Updated

2 years ago
Duplicate of this bug: 1398538
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.