Bug 94622 – Background images can incorrectly repeat with sub-pixel layout

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

We're incorrectly truncating in a few places.

[Levi Weintraub]

Created attachment 159780 [details]
Patch

Patch.

[Eric Seidel (no email)]

Probably best for you to explain this to me in person.

[Levi Weintraub]

(In reply to comment #2)
> Probably best for you to explain this to me in person.

Tomorrow?

[Levi Weintraub]

Ping.

[Eric Seidel (no email)]

Sorry, I missed you before burning man.  Tomorrow, for realz this time.

[Eric Seidel (no email)]

Comment on attachment 159780 [details]
Patch

Levi and I talked about this at length this afternoon.  This fix is essentially trying to follow the intent of the author in the case of zooming, even when we don't have all the necessary information.

The problem starts because background-images by default tile instead of scale, yet authors often specify a background image which is exactly the size of their object.

If we happen to be scaling the page, and size of that object could deviate from the size of the background image by as much as one pixel.

In that case, we'll tile the background image, which be believe is likely not what the author intended.

The fix currently proposed is an attempt to make our background image tiling code match approximately what the rendering tree does with the divs.  This is of course impossible, since he tiles are always drawn of uniform size, and the objects in the rendering tree may gain pixels based on accumulated pixel offsets from the zoom.

I suspect that even with this proposed patch we could still have this problem, as we're using round() here instead of ciel().  ciel() would make it so that we were never too small, but would more often be too big.  The previous code truncated which would make the background image most often too small, and never too big.  round() gives us a little bit of both still. :)

Another approach to this might be to add special code for the painting of background images, which attempted to detect when we're drawing a background image which is one pixel smaller than the div, and specified to tile, and instead change it to scale in that case.  That heuristic would solve this observed problem during zooming, and might be closer to what the authors intend.  It's a heuristic, and would obviously deviate from the CSS spec, but might be the "right" thing to do.

[Eric Seidel (no email)]

Comment on attachment 159780 [details]
Patch

r-, per above.  I also think we could get better testing of this exact case pretty easily which showed the difference between old (floor) behavior, round() and ciel()?  I'm happy to help you come up with such if you like.

[Levi Weintraub]

Created attachment 164611 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 164611 [details]
Patch

I think this looks great.  My only comments would be:

1.  We should make the function name (or at least commetns in the code) make clear that this is a heuristic we're applying (aka, not following any spec, or what you might expect the math to do).

2.  I would limit this to the common case where the tile repeats exactly once, and we're one pixel off.  (Do we need to support either way?  both floor/ and ciel?)  I guess the image is never a subpixel width?

[Levi Weintraub]

Created attachment 171087 [details]
Patch

[Emil A Eklund]

Comment on attachment 171087 [details]
Patch

The updated patch addresses the concerns mentioned earlier and the new test is more to the point. Looks good to me.

[WebKit Review Bot]

Comment on attachment 171087 [details]
Patch

Rejecting attachment 171087 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
content): Merge conflict in LayoutTests/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Pixel tests need rebaseline https://bugs.webkit.org/show_bug.cgi?id=99323

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 154.

Full output: http://queues.webkit.org/results/14610477

[Levi Weintraub]

Comment on attachment 171087 [details]
Patch

Grumble grumble ChangeLogs grumble grumble.

[WebKit Review Bot]

Comment on attachment 171087 [details]
Patch

Clearing flags on attachment: 171087

Committed r132731: <http://trac.webkit.org/changeset/132731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.