Closed Bug 1386576 Opened 2 years ago Closed 2 years ago

Back/forward context menu doesn't show back/forward icons anymore

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + wontfix
firefox56 --- verified
firefox57 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Tracking Requested - why for this release]: recent UI regression
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment on attachment 8892831 [details]
Bug 1386576 - Use list-style-image rather than the image attribute for items in the back/forward menu so that CSS can override this to display back/forward icons.

https://reviewboard.mozilla.org/r/163820/#review169182

I wonder why I changed that originally,
Attachment #8892831 - Flags: review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/696ea3253bc5
Use list-style-image rather than the image attribute for items in the back/forward menu so that CSS can override this to display back/forward icons. r=mak
Attachment #8892831 - Flags: review?(paolo.mozmail)
Backed out for one closing parenthesis too many in browser.js:

https://hg.mozilla.org/integration/autoland/rev/32d12b58e7af284099ad9d2b5ca057f0e8532eb3

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=696ea3253bc539ccaa850baacd7e91c4c93dabe6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120352942&repo=autoland

[task 2017-08-02T12:31:01.068380Z] Could not load globals from file /home/worker/checkouts/gecko/browser/base/content/browser.js: SyntaxError: Unexpected token )
[task 2017-08-02T12:31:01.068456Z] You may need to update the mappings in /home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/browser-window.js
[task 2017-08-02T12:31:01.068541Z] Could not load globals from file /home/worker/checkouts/gecko/browser/base/content/browser.js: SyntaxError: Unexpected token )
[task 2017-08-02T12:31:01.068614Z] Error: Could not load globals from file /home/worker/checkouts/gecko/browser/base/content/browser.js: SyntaxError: Unexpected token )
[task 2017-08-02T12:31:01.068685Z]     at getScriptGlobals (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/browser-window.js:102:13)
[task 2017-08-02T12:31:01.068761Z]     at getMozillaCentralItems (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/browser-window.js:121:25)
[task 2017-08-02T12:31:01.068835Z]     at Object.<anonymous> (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/browser-window.js:127:2)
[task 2017-08-02T12:31:01.068876Z]     at Module._compile (module.js:570:32)
[task 2017-08-02T12:31:01.068924Z]     at Object.Module._extensions..js (module.js:579:10)
[task 2017-08-02T12:31:01.068965Z]     at Module.load (module.js:487:32)
[task 2017-08-02T12:31:01.069009Z]     at tryModuleLoad (module.js:446:12)
[task 2017-08-02T12:31:01.069081Z]     at Function.Module._load (module.js:438:3)
[task 2017-08-02T12:31:01.069125Z]     at Module.require (module.js:497:17)
[task 2017-08-02T12:31:01.069171Z]     at require (internal/module.js:20:19)
Flags: needinfo?(dao+bmo)
Dunno what happened. I somehow must have pushed to review an older revision of my patch. :/ The extra parenthesis completely broke the build and I had definitely fixed and tested this locally.
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/638c4966e064
Use list-style-image rather than the image attribute for items in the back/forward menu so that CSS can override this to display back/forward icons. r=mak
https://hg.mozilla.org/mozilla-central/rev/638c4966e064
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment on attachment 8892831 [details]
Bug 1386576 - Use list-style-image rather than the image attribute for items in the back/forward menu so that CSS can override this to display back/forward icons.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1337858
[User impact if declined]: missing back/forward icons in the back/forward buttons' context menu
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes: open the context menu on the back/forward buttons and hover over the items
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: just reverting a small change that was accidentally made in bug 1337858
[String changes made/needed]: /
Attachment #8892831 - Flags: approval-mozilla-beta?
Attachment #8892831 - Flags: approval-mozilla-release?
https://hg.mozilla.org/projects/date/rev/638c4966e06451c363a2de782666b1e6469b3bf4
Bug 1386576 - Use list-style-image rather than the image attribute for items in the back/forward menu so that CSS can override this to display back/forward icons. r=mak
Comment on attachment 8892831 [details]
Bug 1386576 - Use list-style-image rather than the image attribute for items in the back/forward menu so that CSS can override this to display back/forward icons.

Taking it for beta. Should be in 56 beta 2.
For now, for release, I don't think we should take it. We had the issue for the whole 56 beta cycle and afaik, nobody complaint about this bug.
Should ship with 57 instead.

If you disagree, feel free to reach out to me.
Attachment #8892831 - Flags: approval-mozilla-release?
Attachment #8892831 - Flags: approval-mozilla-release-
Attachment #8892831 - Flags: approval-mozilla-beta?
Attachment #8892831 - Flags: approval-mozilla-beta+
(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Comment on attachment 8892831 [details]
> Bug 1386576 - Use list-style-image rather than the image attribute for items
> in the back/forward menu so that CSS can override this to display
> back/forward icons.
> 
> Taking it for beta. Should be in 56 beta 2.
> For now, for release, I don't think we should take it.

I guess it's too late now anyway. It's irritating that this extremely simple patch was sitting around for six days when presumably it could have landed easily on release during that time.

> We had the issue for
> the whole 56 beta cycle and afaik, nobody complaint about this bug.
> Should ship with 57 instead.

It's not that easy to notice, because the arrows are only really helpful when the menu contains both back and forward items. But when it happens, it can be irritating.
(flags accidentally reset in mid-air)
(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> I guess it's too late now anyway. It's irritating that this extremely simple
> patch was sitting around for six days when presumably it could have landed
> easily on release during that time.
Actually, we saw your bug back then. We (Julien and I) made the call not to take it because rc3 was already started and it would have delayed the 55 release itself.

> > We had the issue for
> > the whole 56 beta cycle and afaik, nobody complaint about this bug.
> > Should ship with 57 instead.
> 
> It's not that easy to notice, because the arrows are only really helpful
> when the menu contains both back and forward items. But when it happens, it
> can be irritating.
irritating a lot or a bit ? :)
(try to see if I should change my mind or not)
(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> > > We had the issue for
> > > the whole 56 beta cycle and afaik, nobody complaint about this bug.
> > > Should ship with 57 instead.
> > 
> > It's not that easy to notice, because the arrows are only really helpful
> > when the menu contains both back and forward items. But when it happens, it
> > can be irritating.
> irritating a lot or a bit ? :)

A bit.
[Tracking Requested - why for this release]:
I tested this bug using Nightly from 2017-08-02, latest Nightly, beta 56.0b2 and beta 56.0b3 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12 and I couldn't reproduce it.

The way I performed the testing was by accessing a site and click on different links, then I right-click and navigated with the arrows from context menu. There wasn't any situation in which the arrows were missing. Maybe I missed something, can you please give me some additional information about how could I reproduce this?
Flags: needinfo?(dao+bmo)
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #18)
> I tested this bug using Nightly from 2017-08-02, latest Nightly, beta 56.0b2
> and beta 56.0b3 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12 and I
> couldn't reproduce it.
> 
> The way I performed the testing was by accessing a site and click on
> different links, then I right-click and navigated with the arrows from
> context menu. There wasn't any situation in which the arrows were missing.
> Maybe I missed something, can you please give me some additional information
> about how could I reproduce this?

This bug is about the back and forward buttons' context menu, not about the content area context menu.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #18)
> > I tested this bug using Nightly from 2017-08-02, latest Nightly, beta 56.0b2
> > and beta 56.0b3 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12 and I
> > couldn't reproduce it.
> > 
> > The way I performed the testing was by accessing a site and click on
> > different links, then I right-click and navigated with the arrows from
> > context menu. There wasn't any situation in which the arrows were missing.
> > Maybe I missed something, can you please give me some additional information
> > about how could I reproduce this?
> 
> This bug is about the back and forward buttons' context menu, not about the
> content area context menu.

Oh yeah, looks like I was missing something after all, sorry for that. Thanks for the clarification!

I retested everything and I managed to reproduce the problem on the Nightly from 2017-08-02 using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12.

I also verified the bug on Beta 56.0b3 and latest Nightly 57.0a1 using all three platforms and the bug seems fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.