1294102 - Button rendering is inconsistent under certain circumstances

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[joel.boonstra]

Attachment: buttons-narrow-and-wide.png

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.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: testcase from comment #0

Comment 2

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4724e966fc73c605cc4865cfb208d5e98c9319d8&tochange=f89869a36b165956bcb3800ed33c65f66aa7c620

it looks like a regression from bug 844948.
xidorn, do you have any idea?

Comment 3

[Markus Stange [:mstange]]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(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?

Comment 6

[Markus Stange [:mstange]]

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.

Comment 7

[Tooru Fujisawa [:arai]]

pushed draft patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a89f7216cc59

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Use the original height that is not affected by the top to decide the button style on OSX.

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).

Comment 9

[Markus Stange [: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.

Comment 10

[Tooru Fujisawa [:arai]]

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

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea4f96dc86d7e497014c5840d05b76dc4b7dd22
Bug 1294102 followup - Fix reftest.list syntax. r=bustage

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0620a588258b
https://hg.mozilla.org/mozilla-central/rev/4ea4f96dc86d

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Seems to be a regression introduced in 46.
Tooru, is it something we should uplift to aurora/beta? Thanks

Comment 14

[Tooru Fujisawa [:arai]]

Yeah, it would be nice to uplift.
will prepare patch for both aurora and beta branches.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Use the original height that is not affected by the top to decide the button style on OSX. r=mstange

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

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Use the original height that is not affected by the top to decide the button style on OSX. r=mstange

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

Comment 17

[Gerry Chang [:gchang]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a353fc4095ca

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/314849c716d7