Bug 14185 – Implement 'round' and 'space' values for border-image

• Component: CSS
• Status: RESOLVED FIXED

[Dave Hyatt]

'round' has little rounding errors.  Have to break up into two pattern tiles in order to avoid this.

[Dave Hyatt]

I forgot that we dropped round support.  Repeat is busted because the removal of round support was botched.

[Dave Hyatt]

Changing this bug to be about just implementing 'round' again period.

[Benjamin Otte]

Created attachment 42575 [details]
patch

Here's a patch.

The test is an adapted border-image-01.html that makes the size of the divs more uneven, so rounding can be properly evaluated.

[Benjamin Otte]

Created attachment 42585 [details]
patch v2

Second try, with nastier testcase.

[Darin Adler]

Comment on attachment 42585 [details]
patch v2

Thanks for tackling this.

>      if (hRule == Image::RepeatTile)
>          scaleX = scaleY;
> +    else if (hRule == Image::RoundTile)
> +        scaleX = dstRect.width() / (srcRect.width() * roundf(dstRect.width() / (srcRect.width() * scaleY)));
> +
>      if (vRule == Image::RepeatTile)
>          scaleY = scaleX;
> +    else if (vRule == Image::RoundTile)
> +        scaleY = dstRect.height() / (srcRect.height() * roundf(dstRect.height() / (srcRect.height() * scaleX)));

I think these should use switch statements now. It helps document that we have cases to cover all values.

Since you're adding a new regression test, you need to include regression test results as well. Also, a good test documents what the result is supposed to look like so people, not just computers, can judge whether it is a success or failure. Ideally tests can be judged for correctness without requiring pixel-test results, but I suppose that may be impossible in this case.

review- because the patch has a new test, but no test results

[Benjamin Otte]

Created attachment 42674 [details]
somewhat bigger patch

So I was annoyed yesterday that the code got the scale factors all wrong and set out to change it. This is the result. As it implements border-image-repeat: round as a side effect, I decided to just put it here.

The code quickly became complicated and I concentrated more on keeping it readable and understandable, so I probably failed to adhere to the Webkit coding standards in places. But it does work.

I also did not add any expected results for the test for a very simple reason: The test is a pixel test, I'm on Linux/gtk and our DRT does not support pixel tests yet. Also, Cairo and CG produce different output for this example; they seem to scale slightly differently.

Last but not least: The patch is done on top of the patch in bug 31187.


I'm now mostly interested in 3 things:
- Are there any bugs in it?
- A rough review of the code style. Are there any refactorings needed?
- What's the best way for me to get test results?

[Alexey Proskuryakov]

Please mark the patch for review if you'd like to have it commented on. Also, please see <http://webkit.org/coding/contributing.html> for more information.

[Alex Yaroshevich]

Good job, Ben. Thank you!)

[Alex Yaroshevich]

That could be implemented 2 years ago. Whats wrong with it?

[Peter Beverloo]

As comment 7 states, the patch wasn't marked for review. This usually means that it's just a WIP.
At the least, the patch needs to be updated to today's codebase before it can be reviewed or commented on, let alone land.

[David Barr]

I noticed this when analyzing https://bugs.webkit.org/show_bug.cgi?id=70871
Border images with 'fill' that would otherwise look fine are terrible without support for 'round'.

[David Barr]

Ben, any chance you could upload a squashed version of this series and be sure to include the svn rev of WebKit that it is based on?

[Simon Fraser (smfr)]

We'll need this to pass CSS Backgrounds and Borders tests.

[Uday Kiran]

Created attachment 188272 [details]
Rebased patch

Patch for EWS run

[Uday Kiran]

Comment on attachment 188272 [details]
Rebased patch

Marking patch for review to get some comments.

[WebKit Review Bot]

Comment on attachment 188272 [details]
Rebased patch

Attachment 188272 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16559385

New failing tests:
fast/borders/border-image-01.html
fast/borders/border-image-rotate-transform.html
fast/borders/scaled-border-image.html
fast/borders/border-image-outset.html
fast/borders/border-image-border-radius.html
fast/backgrounds/mask-box-image.html
fast/borders/inline-mask-overlay-image-outset-vertical-rl.html
fast/borders/border-image-source.html
fast/borders/border-image-scaled.html
fast/borders/inline-mask-overlay-image-outset.html
fast/borders/block-mask-overlay-image-outset.html
fast/css/transformed-mask.html
fast/borders/border-image-slice-constrained.html
fast/forms/datalist/update-range-with-datalist.html
fast/borders/border-image-scaled-gradient.html
fast/borders/border-image-side-reduction.html
fast/borders/border-image-repeat.html
css3/filters/filter-mask-clip-order.html
animations/cross-fade-border-image-source.html
fast/borders/border-image-massive-scale.html
fast/borders/border-image-longhand.html
fast/borders/border-image-outset-split-inline.html
fast/borders/border-image-scrambled.html
fast/borders/block-mask-overlay-image.html
fast/borders/border-image-omit-right-slice.html
fast/borders/border-image-scale-transform.html
fast/borders/border-image-slices.html
fast/borders/border-image-outset-split-inline-vertical-lr.html
fast/borders/border-image-outset-in-shorthand.html
fast/borders/inline-mask-overlay-image.html

[Uday Kiran]

Created attachment 188377 [details]
Updated patch

One more EWS run

[Uday Kiran]

Created attachment 188414 [details]
Updated patch

Fixed failing tests. Added test and test result.

[WebKit Review Bot]

Comment on attachment 188414 [details]
Updated patch

Attachment 188414 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16542785

New failing tests:
fast/borders/border-image-rotate-transform.html
fast/borders/inline-mask-overlay-image-outset-vertical-rl.html
fast/borders/border-image-side-reduction.html
fast/borders/border-image-02.html
fast/borders/inline-mask-overlay-image-outset.html
scrollbars/overflow-scrollbar-combinations.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html

[WebKit Review Bot]

Comment on attachment 188414 [details]
Updated patch

Attachment 188414 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16537829

New failing tests:
fast/borders/border-image-rotate-transform.html
fast/borders/inline-mask-overlay-image-outset-vertical-rl.html
fast/borders/border-image-side-reduction.html
fast/borders/border-image-02.html
fast/borders/inline-mask-overlay-image-outset.html
scrollbars/overflow-scrollbar-combinations.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html

[Uday Kiran]

Created attachment 188478 [details]
Updated patch

Fixed failing tests. border-image-rotate-transform.html test needs to be regenerated.

[Build Bot]

Comment on attachment 188478 [details]
Updated patch

Attachment 188478 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16570746

New failing tests:
fast/borders/border-image-02.html

[Uday Kiran]

(In reply to comment #22)
> (From update of attachment 188478 [details])
> Attachment 188478 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16570746
> 
> New failing tests:
> fast/borders/border-image-02.html

I will update TestExpectations in patch after review comments.

[Uday Kiran]

Created attachment 188641 [details]
Updated patch

Updated TestExpectations for other platforms.

[Uday Kiran]

Created attachment 188644 [details]
Rebased patch

Updated TestExpectations for other platforms.

[Build Bot]

Comment on attachment 188644 [details]
Rebased patch

Attachment 188644 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16600663

New failing tests:
inspector/styles/stylesheet-tracking.html

[Dave Hyatt]

Comment on attachment 188644 [details]
Rebased patch

This looks good. r=me

I think we should perhaps seek clarification regarding painting order of border images. My instinct would be that the stacking order would be that we would paint the middle, then the sides, and finally the corners on top. In the case of overlap, I'd expect the middle to be underneath the side edges and corners, and I'd expect the sides to be underneath the corners. Might be worth filing a follow-up bug about that.

When you do land this, watch the tree and be on IRC in case this causes regressions that necessitate a rollout.

[Dave Hyatt]

Oh one other thing. This is obviously not the best implementation of "round," since it's going to be very prone to pixel cracking. The reason I never implemented "round" is because doing a really good implementation is pretty involved. You can't get away with just doing one tiling pass. Ideally you want all the tiles to be integral widths, which means working out a strategy for how to distribute the extra space to the tiles to make that happen.

[Dave Hyatt]

So basically I'm saying we should file a follow-up bug for working out the paint order and another follow-up bug for improving the rounding algorithm.

[Uday Kiran]

Created attachment 188991 [details]
Patch for landing

[WebKit Review Bot]

Attachment 188991 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Uday Kiran]

Created attachment 188993 [details]
Patch for landing

[WebKit Review Bot]

Attachment 188993 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Uday Kiran]

Created attachment 188995 [details]
Patch for landing

[WebKit Review Bot]

Attachment 188995 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Uday Kiran]

(In reply to comment #35)
> Attachment 188995 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1
> LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
> Total errors found: 1 in 17 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This seems strange even though my subversion config file has properties set.

[Uday Kiran]

Created attachment 189000 [details]
Patch for landing

[WebKit Review Bot]

Attachment 189000 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Uday Kiran]

style error seems like bug in bot https://bugs.webkit.org/show_bug.cgi?id=107724

[Uday Kiran]

(In reply to comment #27)
> (From update of attachment 188644 [details])
> This looks good. r=me
> 
> I think we should perhaps seek clarification regarding painting order of border images. My instinct would be that the stacking order would be that we would paint the middle, then the sides, and finally the corners on top. In the case of overlap, I'd expect the middle to be underneath the side edges and corners, and I'd expect the sides to be underneath the corners. Might be worth filing a follow-up bug about that.
> 
https://bugs.webkit.org/show_bug.cgi?id=110185

>follow-up bug for improving the rounding algorithm.
https://bugs.webkit.org/show_bug.cgi?id=110188

> When you do land this, watch the tree and be on IRC in case this causes regressions that necessitate a rollout.

Sure I will try to land this tomorrow and check the tree if it causes any regressions. I will be on IRC too (:ukr)
Hopefully no regression :)

Thanks for review.

[WebKit Review Bot]

Comment on attachment 189000 [details]
Patch for landing

Rejecting attachment 189000 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189000, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
um/third_party/v8-i18n --revision 164 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 164.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
Total errors found: 0 in 6 files

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

[Uday Kiran]

Created attachment 189148 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189148 [details]
Patch for landing

Rejecting attachment 189148 [details] from commit-queue.

New failing tests:
platform/chromium/fast/forms/calendar-picker/calendar-picker-appearance-required.html
Full output: http://queues.webkit.org/results/16636103

[Uday Kiran]

Comment on attachment 189148 [details]
Patch for landing

Failing test is unrelated to mine. Setting cq? again.

[WebKit Review Bot]

Comment on attachment 189148 [details]
Patch for landing

Clearing flags on attachment: 189148

Committed r143419: <http://trac.webkit.org/changeset/143419>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110507

[Simon Fraser (smfr)]

Created attachment 264032 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 264032 [details]
Patch

Clearing flags on attachment: 264032

Committed r191590: <http://trac.webkit.org/changeset/191590>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/23405291>