Closed Bug 1242271 Opened 5 years ago Closed 5 years ago
Buttons in overflow panel have unnecessary margin-top
>>> 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
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
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: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8743986 - Flags: review?(gijskruitbosch+bugs) → review+
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?
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.
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.
(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.
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.