Closed
Bug 1197652
Opened 9 years ago
Closed 5 years ago
Bookmarks Toolbar Items have no icon in Customization mode if they are placed in Navigation toolbar
Categories
(Firefox :: Theme, defect)
Tracking
()
People
(Reporter: arni2033, Assigned: dao)
References
Details
(Keywords: regression, Whiteboard: fixed by bug 1391017)
Attachments
(2 files)
67.71 KB,
image/png
|
Details | |
3.00 KB,
patch
|
dao
:
review-
|
Details | Diff | Splinter Review |
STR: (Win7, Nightly 43.0a1 (2015-08-20)) 1. Click (≡)->Customize 2. Drag Bookmarks Toolbar Items in Navigation toolbar (near urlbar) Result: Bookmarks Toolbar Items have no icon Expectations: Bookmarks Toolbar Items should have icon
Updated•9 years ago
|
Component: Toolbars and Customization → Theme
Keywords: regression,
regressionwindow-wanted
Updated•9 years ago
|
Severity: normal → trivial
Comment 1•9 years ago
|
||
Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f16cb820431b&tochange=27061dc242e4 Suspect: +0000 69f04d5d6700 Gijs Kruitbosch — Bug 971956 - fix styling of items in widget overflow list, r=mconley
Updated•9 years ago
|
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox-esr31:
--- → affected
status-firefox-esr38:
--- → affected
Updated•9 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 2•9 years ago
|
||
This is happening on windows and linux, but not OSX. The reason is that the width of the icon is set to 16px for all toolbars, and then the "normal" padding and margin for the navbar and/or tabs toolbar is applied (this doesn't happen on the bookmarks toolbar) which doesn't leave enough room for the icon itself. There are issues with the margin and padding as well. I'm trying to figure out what the most straightforward fix is.
Comment 3•9 years ago
|
||
Tested on win8 and Linux, this seems to wfm and be a relatively simple and workable solution.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8652263 -
Flags: review?(dao)
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8652263 [details] [diff] [review] Patch v1 I don't understand why we set the toolbarbutton-1 class on the placeholder. It's quite different from other toolbar buttons; it has a label and it's not supposed to be interactive. Looking at bug 971956, from what I can tell the real bug there is that the placeholder is displayed at all in the overflow menu.
Attachment #8652263 -
Flags: review?(dao) → review-
Comment 5•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #4) > Comment on attachment 8652263 [details] [diff] [review] > Patch v1 > > I don't understand why we set the toolbarbutton-1 class on the placeholder. > It's quite different from other toolbar buttons; it has a label and it's not > supposed to be interactive. Looking at bug 971956, from what I can tell the > real bug there is that the placeholder is displayed at all in the overflow > menu. Removing the class will break third-party themes and require a much larger patch + verification, ensuring it continues to look reasonable in the palette, on the panel, and on all 4 default toolbars, on all platforms. I don't think the severity of this bug warrants making such a large change. What's actually wrong with the smaller fix which addresses the issue at hand?
Flags: needinfo?(dao)
Assignee | ||
Comment 6•9 years ago
|
||
The problem with the patch is that it builds on something broken, making the whole thing even weirder and harder to maintain if (God forbid) we need to touch it again. The placeholder wasn't originally intended to be displayed in the panel. Its label doesn't even make sense there. Maybe we should have another button that would be displayed specifically in that case? This should be straightforward and shouldn't need extraordinary verification efforts. I see no difference at all in the palette when removing the class. Am I missing something?
Flags: needinfo?(dao)
Comment 7•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #6) > The problem with the patch is that it builds on something broken, making the > whole thing even weirder and harder to maintain if (God forbid) we need to > touch it again. I don't think removing the specialcasing of this button in 2 places, adding a single rule next to an existing one specific to this button, and adding 1 property to that existing rule, really makes a material maintenance difference. > The placeholder wasn't originally intended to be displayed in the panel. Its > label doesn't even make sense there. What's wrong with the label being "Bookmark toolbar items" in the panel? I don't follow. > Maybe we should have another button > that would be displayed specifically in that case? This should be > straightforward and shouldn't need extraordinary verification efforts. In my mind, that is a lot less straightforward than the patch currently on this bug. > I see no difference at all in the palette when removing the class. Am I > missing something? My point was about the increase in scope, which is significant, rather than about the specifics of that increase in scope. Maybe we don't need to touch the palette styling, but we do need to add a separate button to make it work in the panel(s) ? That's a lot more work. I don't think the severity of this bug justifies that work.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #7) > > The placeholder wasn't originally intended to be displayed in the panel. Its > > label doesn't even make sense there. > > What's wrong with the label being "Bookmark toolbar items" in the panel? I > don't follow. IMHO it should be either "Bookmarks Toolbar" and open a subview or just "Bookmarks" and open all bookmarks in the library. I think the former is preferable as it stays true to the real bookmarks toolbar's spirit (allowing you to pick from a more easily accessible subset of your bookmarks), but the latter is probably much simpler. The current implementation feels half-assed and rushed, which in fact it probably is given the pressure to ship Australis... > That's a lot more work. I don't think the severity of this > bug justifies that work. It probably doesn't. It would be a maintenance effort that hopefully will pay off beyond just fixing this particular bug. I'll give it a shot.
Assignee: nobody → dao
Comment 9•8 years ago
|
||
I can not reproduce this issue on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 I will close this issue as wfm. Please reopen if you can still reproduce.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Comment 10•8 years ago
|
||
(In reply to Justin [:JW_SoftvisionQA] from comment #9) > I can not reproduce this issue on Mozilla/5.0 (Windows NT 6.1; WOW64; > rv:48.0) Gecko/20100101 Firefox/48.0 > > I will close this issue as wfm. Please reopen if you can still reproduce. I can still reproduce on nightly and release.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment 11•8 years ago
|
||
Whoops, I was looking at the wrong icon. This is also reproducible on Win 8.1 and Win 10.
Comment 12•6 years ago
|
||
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 6 years ago
Resolution: --- → INACTIVE
Assignee | ||
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Assignee | ||
Comment 13•5 years ago
|
||
I believe bug 1391017 fixed this.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 5 years ago
Depends on: 1391017
Resolution: --- → FIXED
Whiteboard: fixed by bug 1391017
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•