Closed Bug 1242271 Opened 5 years ago Closed 5 years ago

Buttons in overflow panel have unnecessary margin-top

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox46 --- affected
firefox48 --- fixed

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
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: 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.
Attachment #8743986 - Attachment is obsolete: true
Attachment #8744082 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8969263da896
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
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.
(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.
Depends on: 1270495
See Also: → 1327535
You need to log in before you can comment on or make changes to this bug.