Button rendering is inconsistent under certain circumstances

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: joel.boonstra, Assigned: arai)

Tracking

({regression})

48 Branch
mozilla51
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox-esr45 unaffected, firefox50 fixed, firefox51 fixed)

Details

Attachments

(5 attachments)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160726073904

Steps to reproduce:

Given HTML and CSS like this: https://gist.github.com/jboonstra/3f84f0bebb5fc228ad096cb4f6658930

the rendering of each button changes based on the line on the screen that it appears on. Resizing the browser window to be narrower or wider will cause buttons to wrap to a new line and change their appearance based on the line they're on. The rendering pattern appears to go in an ABABA repeating pattern.

If any of the three CSS values (font-size, line-height, padding) changes or is removed entirely, the buttons all render consistently regardless of the line they are on.

To reproduce, create an HTML page containing the HTML contained in the gist above, open it in Firefox (either as a local file or via http) and resize the browser window to be very narrow or very wide, using either the window itself or responsive design view.




Actual results:

The button rendering changes based on how wide the browser window is. See attached screenshots for various widths. The pattern of rendering follows an ABABA pattern (a line of white buttons, then gray, then white, then gray, then white, then it starts over again with white/gray/white etc.).


Expected results:

I would expect the buttons to render identically regardless of the line they are on; either all using the "gray" style or all using the "white" style.
OS: Unspecified → Mac OS X
Status: UNCONFIRMED → NEW
Component: Activity Streams: General → Layout: Form Controls
Ever confirmed: true
Product: Firefox → Core
We compute the size and position of the button at a subpixel resolution. Then, when the button gets rendered, we snap the button rect to screen pixels. If the button has a fractional height (in terms of screen pixels), then the height of the snapped rectangle can vary by 1 pixel. For example:
y=0.1 height=10.3 (bottom = 10.4) => snapped_y=0, snapped_bottom=10 => snapped_height=10
y=0.3 height=10.3 (bottom = 10.6) => snapped_y=0, snapped_bottom=11 => snapped_height=11

Unfortunately, the decision between the button styles is based on snapped_height. So if the true, fractional height is exactly around the threshold, then the style can depend on the fractional position of the button.

I agree that it's unfortunate, but I don't really know what we should do about it.

Bug 844948 probably somehow caused the button height to be a fractional value, or caused the height to increase so that it's around the button style threshold.
Actually this is not a regression from bug 844948. If you change the style to:
> button {
>   font-size: 15.5px;
>   line-height: 1.3;
> }
or
> button {
>   font-size: 14px;
>   line-height: 1.45;
> }
You would see the same thing in earlier versions.

What bug 844948 did is just enabling padding on buttons, however padding is just one of many things which contribute to the height.
Flags: needinfo?(xidorn+moz)
(In reply to Markus Stange [:mstange] from comment #3)
> I agree that it's unfortunate, but I don't really know what we should do
> about it.

Could we instead, snap the top edge to screen pixels first, then snap the bottom edge based on top + height?
That would probably work for buttons in HTML, but not for things where adjacent edges should stay adjacent. For example XUL tree header cells, or segmented toolbar buttons like in the page info dialog.

But what we could do is: pass the unsnapped true height through to this line http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/widget/cocoa/nsNativeThemeCocoa.mm#1228 and then decide the button style based on that unsnapped true button height. In the case where the unsnapped height is smaller than the threshold but the snapped height is larger, this would just cause DrawCellWithSnapping to add more spacing at the top or bottom, which should be fine.
At first, I was about to use the truly unsnapped height, but it results in
using square corner button for, e.g., 26.3px height, as it's greater than DO_SQUARE_BUTTON_HEIGHT=26.

now I'm using rounded true height (26px for 26.3px), so that the rendering result of 26.3px height button keeps same rendering for non-fractional positioned buttons.


Then, about the testcase, it's hard to prepare reference case that is exactly same rendering for the this bug's case,
as fractional part affects the height, and even it keeps same style (square or rounded corner), the height of the button is different.

so in the patch I'm comparing the style only, and ignoring the size.
if the true height is used for 26.3px height button, it should be always rounded corner button that has no gradient background.
if the snapped height is used for 26.3px height button, it may use square corner button that has gradient background.
the testcase checks if the rounded corner button is used, by allowing the difference in border (that should be less than 1000 pixels), but not allowing the difference in background (that should be more than 1000 pixels).
Assignee: nobody → arai.unmht
Attachment #8780089 - Flags: review?(mstange)
Comment on attachment 8780089 [details] [diff] [review]
Use the original height that is not affected by the top to decide the button style on OSX.

Review of attachment 8780089 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

::: widget/cocoa/nsNativeThemeCocoa.mm
@@ +1194,5 @@
>  
>  void
>  nsNativeThemeCocoa::DrawPushButton(CGContextRef cgContext, const HIRect& inBoxRect,
>                                     EventStates inState, uint8_t aWidgetType,
> +                                   nsIFrame* aFrame, float originalHeight)

I know that this file is really inconsistent about argument names, but let's at least follow the mozilla style guide on new arguments. So please make this aOriginalHeight.
Attachment #8780089 - Flags: review?(mstange) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0620a588258bb492c3f29a157f10b9942ef0a30b
Bug 1294102 - Use the original height that is not affected by the top to decide the button style on OSX. r=mstange
https://hg.mozilla.org/mozilla-central/rev/0620a588258b
https://hg.mozilla.org/mozilla-central/rev/4ea4f96dc86d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Seems to be a regression introduced in 46.
Tooru, is it something we should uplift to aurora/beta? Thanks
Flags: needinfo?(arai.unmht)
Keywords: regression
Yeah, it would be nice to uplift.
will prepare patch for both aurora and beta branches.
Prepared folded patches for aurora and beta.
no difference from m-c's patches except surrounding lines in reftest.list.

Approval Request Comment
> [Feature/regressing bug #]:
844948

> [User impact if declined]:
Inconsistent rendering of buttons with same size, depends on the position.

> [Describe test coverage new/current, TreeHerder]:
Tested on m-c

> [Risks and why]:
Low, just passing more accurate value.

> [String/UUID change made/needed]:
None
Attachment #8784047 - Flags: review+
Attachment #8784047 - Flags: approval-mozilla-aurora?
this too, no difference from m-c's patches except surrounding lines in reftest.list.

Approval Request Comment
> [Feature/regressing bug #]:
844948

> [User impact if declined]:
Inconsistent rendering of buttons with same size, depends on the position.

> [Describe test coverage new/current, TreeHerder]:
Tested on m-c

> [Risks and why]:
Low, just passing more accurate value.

> [String/UUID change made/needed]:
None
Flags: needinfo?(arai.unmht)
Attachment #8784048 - Flags: review+
Attachment #8784048 - Flags: approval-mozilla-beta?
Comment on attachment 8784048 [details] [diff] [review]
(mozilla-beta) Use the original height that is not affected by the top to decide the button style on OSX. r=mstange

Review of attachment 8784048 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a button rendering regression. Take it in aurora and 49 beta. Should be in 49 beta 7.
Attachment #8784048 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8784047 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.