Closed
Bug 1242271
Opened 8 years ago
Closed 8 years ago
Buttons in overflow panel have unnecessary margin-top
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 48
People
(Reporter: arni2033, Assigned: jaws)
References
Details
Attachments
(3 files, 1 obsolete file)
>>> My Info: Win7_64, Nightly 46, 32bit, ID 20160121030208
STR:
1. Remove all wide items from Toolbar to Menu, move at least 5 normal buttons to Toolbar
2. Switch window to normal mode, resize it to be as narrow as possible
3. Click overflow button in Toolbar, hover mouse over the 1st button in tooltip
Result: The button has unnecessary margin-top
Expectations: 1st item in overflow tooltip shouldn't have margin-top it it's normal button
(normal button == button with class .toolbarbutton-1)
Note that wide items (Zoom level widget, Searchbar, Cut-Copy-Paste) don't have that margin-top
Assignee | ||
Comment 1•8 years ago
|
||
I'm almost certain this because each overflowed toolbaritem has a margin-top and the panel itself has a padding-top. We should remove the margin-top of the first toolbaritem child.
Summary: Buttons in overflow tooltip have unnecessary margin-top → Buttons in overflow panel have unnecessary margin-top
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48223/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48223/
Attachment #8743986 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Updated•8 years ago
|
Attachment #8743986 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 3•8 years ago
|
||
Comment on attachment 8743986 [details] MozReview Request: Bug 1242271 - The first item in the overflow panel doesn't need a margin-top as the panel has its own padding-top. r?gijs https://reviewboard.mozilla.org/r/48223/#review44937 Nice. Do we need to add margin to non-first wide items?
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/48223/#review44937 Yeah, good catch. I've added that locally and confirmed that it looks better now when hovering between items. We don't show a border on the top of the wide items, so it does look like there is a larger space now when it is not hovered.
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8743986 -
Attachment is obsolete: true
Attachment #8744082 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8969263da896
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I reported this on Win7. Did somebody actually test the fix on Win7? A thing irrelevant to my report was broken by adding unnecessary out-of-nowhere brand new stuff. This could be done in separate bug.
Comment 9•8 years ago
|
||
(In reply to arni2033 from comment #8) > I reported this on Win7. Did somebody actually test the fix on Win7? It was tested on Windows; I don't know if it was tested specifically on Windows 7 - I didn't. There is not much (if any...) difference between Windows 7 and later styling as far as spacing in panels is concerned, so either way I don't really know why you think testing on Windows 8 or 10 wouldn't be sufficient for this particular patch. Certainly the original issue was not specific to Windows 7 or even Windows. > A thing irrelevant to my report was broken I looked, but I didn't see any bugs filed by you since this comment. What's broken? Because you haven't explained what "thing" was "broken", your comment is not actionable and seems to just be passive-aggressive complaining. That's not very nice; please don't do that, and file a new bug instead, or at least include *what* is broken when commenting here. > by adding unnecessary out-of-nowhere That's an unfair characterization. The addition of spacing for the wide items was pretty much implied by comment #0 saying: "Note that wide items (Zoom level widget, Searchbar, Cut-Copy-Paste) don't have that margin-top" -- it seemed clear that the spacing was uneven and that could be fixed by broadening the selector in question, so we did.
Reporter | ||
Comment 10•8 years ago
|
||
Well, yes. That wasn't "out of nowhere". I must say, I became far less attentive after a year, but at least I can still detect bugs correctly. I haven't said anything first on purpose, because it seemed nobody even cared to test, given the issue looks too obvious yet minor. So I was quite disappointed. On the screenshot there's red arrow and also the fact that the last 2 big widgets aren't separated by 6px indent, as the patch was going to do. This is clearly a mistake in second selector in the patch.
You need to log in
before you can comment on or make changes to this bug.
Description
•