Bug 101848

Created attachment 173467 [details] After r133351

Levi?

Created attachment 173469 [details] Before r133351

<rdar://problem/12678254>

This is not specific to bugs.webkit.org. All standard buttons are broken in the same fashion.

Interestingly, while <input type='button' renders the text off by one pixel, <button> has the text at the correct position. The sub-pixel layout rounding at InlineTextBox::paint() pushes the text 'down' by one pixel. The reason why the text position stays the same after enabling sub-pixel for <button> but not for <input type='button' is that they use different top/bottom paddings. With <button>'s padding no rounding happens at http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/InlineTextBox.cpp#L505 <input type='button' has hardcoded values for padding (top:0x, bottom:0px) while <button> applies the default style values(top:2px bottom: 3px -html.css). Normal button size is 18px (hardcoded again) and the line spacing (default font) for the text is 13px. With 0px padding, the content position is calculated as (18px-13px)/2 at RenderFlexibleBox::updateAutoMarginsInCrossAxis(), which, after the roundedIntPoint() call at InlineTextBox::paint(), ends up 2px for non-sub-pixel and 3px for sub-pixel. Hence the off by one pixel regression. Before sub-pixel: <input type=button -> padding-top/bottom: 0px -> calculated 2px off of top border <button> -> padding-top:2px, padding-bottom:3px, same, 2px adjusted text position result With sub-pixel: <input type=button -> padding-top/bottom: 0px -> rounded 3px off of top border <button> -> padding-top:2px, padding-bottom:3px off by one for <input type=button 1, Removing the hard-coded padding values fixes the off by one issue, but it makes the <input type=button look different when user css defines padding other than the default. (/LayoutTests/fast/forms/basic-buttons.html) 2, Hardcoding it to top:2px bottom:3px also fixes the text placement (and it even works for the different control sizes(hardcoded): 21,15,13) but highly error prone. 3, As per Darin's suggestion, fix the rounding to round the pixel value 'up' vertical direction instead of 'down'.

Can we try my suggestion (3) above? Should be simple enough if we can find where the rounding is happening.

(In reply to comment #7) > Can we try my suggestion (3) above? Should be simple enough if we can find where the rounding is happening. rounding is happening at http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/InlineTextBox.cpp#L505

It’s unfortunate that the rounding is in code shared by all text rendering rather than in code specific to the buttons. But I think that rounding up and to the left would likely be better than down and to the right. We could start by patching InlineTextBox::paint, if necessary, in an ugly way, to round the other direction, and see what test results that affects. Where does the paint offset come from, in this case, by the way? Maybe there’s an opportunity to round there too. I’d like to know what Hyatt thinks.

(In reply to comment #9) > It’s unfortunate that the rounding is in code shared by all text rendering rather than in code specific to the buttons. But I think that rounding up and to the left would likely be better than down and to the right. We could start by patching InlineTextBox::paint, if necessary, in an ugly way, to round the other direction, and see what test results that affects. > > Where does the paint offset come from, in this case, by the way? Maybe there’s an opportunity to round there too. This particular offset value is calculated at http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp#L854 where the alignment space is distributed. (18px-13px)/2 ->2.5px

I think in this case patching flexible box is the better thing to do (rather than patching InlineTextBox::paint). In this particular case we're doing center alignment, and for center alignment I think you would rather put the extra space on the "after" side of the box. It's RenderFlexibleBox that knows what is happening here, so I think it's RenderFlexibleBox that should be patched. This kind of gets into all the subtle issues with subpixel layout not patching the layout systems themselves. Basically the missing piece of subpixel layout that isn't implemented is having the various layout systems like flexbox, grid, tables and blocks make smart decisions about how stuff should be rounded based off how those systems are placing their children.

So a problem here is that by the time we get to the level that rounds to a pixel, InlineTextBox::paint, we have lost the semantic information about which direction we should round. One way to deal with that is to round an extra time, at the higher level, in RenderFlexibleBox, in the appropriate direction. A second is to subtract a small epsilon constant in RenderFlexibleBox to bias the rounding direction A third is to pass information in to InlineTextBox::paint about what direction we should round. As a short term strategy, I like the “subtract a small epsilon constant” trick. Longer term is would be good to get a clearer idea of what our rounding strategy is. Maybe Hyatt can guide us about what levels should be responsible for dong the rounding, longer term.

Well, I made the mistake of composing a long response in ToT, and it crashed when I hit Commit, so this will be brief. We just need to patch "centering" layout subsystems to fix the centering issues. Division by 2 without pre-emptive rounding to push up and to the left is wrong, and we just need to fix that. This includes RenderFlexibleBox, RenderDeprecatedFlexibleBox, line-height leading, text-align, table vertical alignment, auto margins on blocks, etc. For the underline issue that Dan also mentioned, we need to make sure the placement of overline, strike-through and underlines are relative to the rounded position of the drawn text and not just trying to round independently from some source point. That's how errors would creep in for that, so it's easy to fix also.

I think we can make a helper function (not sure exactly where to put it) that will give you a rounded center position, and then we can just patch everyone to use that function.

Thinking about this some more, I'm not sure it's as simple as I made it out to be. The parent flexible box could be at a non-integral position, and maybe the non-integral centering has a final result that puts the child at an integral position. Or perhaps the child has non-integral padding but the end result is that the text gets placed at an integral position. In other words, attempting to snap without knowing the absolute offset is probably not correct. Painting does know the absolute offset, so it can get away with pixel snapping. It may be that rounding during layout is still correct, but local rounding does seem a bit suspicious, since one could obviously design a page with non-integer values that would not need any rounding to occur for the text that you render. Local rounding would then make that page incorrect.

Basically layout needs to know absolute offsets and really know where it is placing stuff in an absolute sense. The other big issue with subpixel layout of course is that we need to know about device pixels as well. It's silly to do rounding to CSS pixels only without taking device pixels into account.

I have an idea for a much simpler way to solve the immediate problem. We can still do what Hyatt suggests longer term to get all sorts of other cases right. But I realize that the problem here comes from the fact that when we computed coordinates before, we always truncated, which meant rounding down the 0.5 case. Now we round 0.5 up to 1. But why is that important? I think we can change our rounding to round 0.5 down to 0 globally and get good results. That would fix this problem right away. Later we could get more sophisticated about rounding, and round up when we need to and down when we need to. But just changing the 0.5 rounding globally would fix this problem and likely cause no serious other problems. I think that’s an experiment we could do quickly that could save us a lot of time. The function we would change is LayoutUnit::round and we’d change it to add kEffectiveFixedPointDenominator / 2 - 1 and subtract kEffectiveFixedPointDenominator / 2 rather than the other way around. I think that this is the right thing to do to fix this bug and it will be a lot faster than rationalizing the rounding logic the way Hyatt proposes, which is still something we should do longer term. Also, as a side note, we should be changing things so we round to pixel boundaries, not integer boundaries. No reason I can think of to round to multiples of 2 pixels on a 2X HiDPI display.

(In reply to comment #17) > But I realize that the problem here comes from the fact that when we computed coordinates before, we always truncated, which meant rounding down the 0.5 case. Now we round 0.5 up to 1. But why is that important? I think we can change our rounding to round 0.5 down to 0 globally and get good results. > > That would fix this problem right away. I had this exact thought and think it's an approach worth trying.

(In reply to comment #17) > Also, as a side note, we should be changing things so we round to pixel boundaries, not integer boundaries. No reason I can think of to round to multiples of 2 pixels on a 2X HiDPI display. I agree in principle, but hit testing is still done in CSS pixels, so rounding to device pixels might add some ambiguity to testing on the edge.

(In reply to comment #15) > Thinking about this some more, I'm not sure it's as simple as I made it out to be. The parent flexible box could be at a non-integral position, and maybe the non-integral centering has a final result that puts the child at an integral position. Or perhaps the child has non-integral padding but the end result is that the text gets placed at an integral position. > > In other words, attempting to snap without knowing the absolute offset is probably not correct. Painting does know the absolute offset, so it can get away with pixel snapping. > > It may be that rounding during layout is still correct, but local rounding does seem a bit suspicious, since one could obviously design a page with non-integer values that would not need any rounding to occur for the text that you render. Local rounding would then make that page incorrect. Agree, rounding blindly certainly works for cases like centering <button>'s text or anything with theming , but not necessarily for the generic flexible box case. I'll go with Darin's suggestion and check how much it breaks edge hit testing.

Thinking about this some more, I don't think it's a good solution to just flip the rounding blindly. Fundamentally what is going on here is that we need to bias rounding of coordinates based off the characteristics of the coordinate space that you're in. Snapping should be biased based off whether or not you're LTR or RTL and also on whether or not you're a flipped block. For example, in horizontal-bt, you want the extra pixel to be on the top when centering. For horizontal-tb, that extra pixel should be on the bottom. Fundamentally the rounding needed to know about the characteristics of the coordinate space. Same goes for LTR and RTL. When centering LTR or RTL you want to make a rounding decision that biases towards the correct direction. I think it basically works out as follows: For a line direction coordinate: LTR - Floor 0.5. Bias is to push left. RTL - Ceiling 0.5. Bias is to push right. For a block direction coordinate: Normal - Floor 0.5. Bias towards the before side of the block. Flipped - Ceiling 0.5. Bias towards the after side of the block.

Anyway, Zalan, experiment with changing the rounding to just floor 0.5 (since that is the correct bias for LTR and top-to-bottom), and let's at least see if it fixes the issues.

*** Bug 111280 has been marked as a duplicate of this bug. ***

This was fixed in 127640; marking as dupe of that. *** This bug has been marked as a duplicate of bug 127640 ***