shape-outside: polygon with shape-margin > 0 doesn't generate all needed intervals if the shape is offset from its containing block

RESOLVED FIXED in Firefox 62

Status

()

defect
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jfkthame, Assigned: bradwerth)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments)

See testcase. When shape-margin is added to the float elements, the ones with float:left appear as expected, but those with float:right are half-overprinted by the text.
Assignee: nobody → bwerth
This problem isn't limited to vertical writing modes. The PolygonShapeInfo constructor, in the shape-margin > 0 path, is miscalculating the block start and end points, which causes it to not generate intervals for some of the block pixel rows. If the polygon is offset within its containing block, the math is off. This isn't being hit in existing WPT reftests because those tests use shapes that have no offset to their containing block.

I have a patch posted which fixes the math. I'll add another part that adds tests for polygon and other shapes where the shapes are offset relative to their containing block.
Summary: shape-margin with polygon fails for element with float:right → shape-outside: polygon with shape-margin > 0 doesn't generate all needed intervals if the shape is offset from its containing block
Attachment #8974536 - Flags: review?(jfkthame)
Attachment #8974198 - Flags: review?(jfkthame)
Priority: -- → P2
Comment on attachment 8974198 [details]
Bug 1460041 Part 1: Correct PolygonShapeInfo constructor to measure its start and end block extents in margin rect space.

https://reviewboard.mozilla.org/r/242490/#review248906
Attachment #8974198 - Flags: review?(jfkthame) → review+
Comment on attachment 8974536 [details]
Bug 1460041 Part 2: Add WPT reftests for many shape-outside shapes that use an element offset from its container.

https://reviewboard.mozilla.org/r/242870/#review248908
Attachment #8974536 - Flags: review?(jfkthame) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1dbfe37baf3
Part 1: Correct PolygonShapeInfo constructor to measure its start and end block extents in margin rect space. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/b19f3977715c
Part 2: Add WPT reftests for many shape-outside shapes that use an element offset from its container. r=jfkthame
 Backed out 2 changesets (bug 1460041) for linting failure at css/vendor-imports/mozilla/mozilla-central-reftests/shapes1 e.g. shape-outside-circle-056-ref.html 

Backout: https://hg.mozilla.org/integration/autoland/rev/d9db16ca4471aab712d5b57448bf1967c6e48916

Failure push : https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b19f3977715cc2b3bb2fd05ac8d6323cedbeb4a1

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177898919&repo=autoland&lineNumber=283

[task 2018-05-10T16:56:24.825Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-05-10T16:56:24.841Z] No specific files specified, running the full wpt lint (this is slow)
[task 2018-05-10T16:56:37.854Z]  0:13.02 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/mozilla/meta/MANIFEST.json
[task 2018-05-10T16:56:37.861Z]  0:13.02 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json
[task 2018-05-10T16:56:40.014Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-circle-056-ref.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.016Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-circle-056.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.016Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-ellipse-052-ref.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.016Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-polygon-032-ref.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.016Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-polygon-032.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.016Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-030.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.017Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/css-shapes/shape-outside/shape-image/shape-image-026.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.017Z]  0:15.18 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-ellipse-052.html in source but not in manifest.  (wpt-manifest)
[task 2018-05-10T16:56:40.209Z]  0:15.37 ERROR Manifest /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json is outdated, use |mach wpt-manifest-update| to fix.
[task 2018-05-10T17:00:20.121Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-circle-056-ref.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.122Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-circle-056.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.122Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-ellipse-052-ref.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.122Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-polygon-032-ref.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.122Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-polygon-032.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.122Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-030.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.122Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/css-shapes/shape-outside/shape-image/shape-image-026.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.123Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-ellipse-052.html in source but not in manifest. (wpt-manifest)
[task 2018-05-10T17:00:20.123Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/lint/wpt.yml:None | Lint process exited with return code 2 (wpt)
[task 2018-05-10T17:00:20.123Z] TEST-UNEXPECTED-ERROR | testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-ellipse-052.html:None | Reference test with a non-existent 'match' relationship reference: 'reference/shape-outside-ellipse-052-ref.html' (NON-EXISTENT-REF)
[task 2018-05-10T17:00:20.123Z] TEST-UNEXPECTED-ERROR | testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-polygon-032.html:None | Reference test with a non-existent 'match' relationship reference: 'reference/shape-outside-polygon-032-ref.html' (NON-EXISTENT-REF)
Flags: needinfo?(bwerth)
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10954 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/w3c/web-platform-tests/pull/10954
* continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/377376808?utm_source=github_status&utm_medium=notification)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f13ab5de9be
Part 1: Correct PolygonShapeInfo constructor to measure its start and end block extents in margin rect space. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/25c0ace5ecb4
Part 2: Add WPT reftests for many shape-outside shapes that use an element offset from its container. r=jfkthame
Backed out 2 changesets (bug 1460041) for lint failure at /builds/worker/checkouts/gecko/tools/lint/wpt.yml on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/autoland/rev/9f13413ebd3ce28b7d9ee4a7e3a73c01c36a0171

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=25c0ace5ecb425acd4f65a5e951b689bbfb2866b&selectedJob=177953441

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177953441&repo=autoland&lineNumber=291

[task 2018-05-10T22:15:22.752Z] No specific files specified, running the full wpt lint (this is slow)
[task 2018-05-10T22:15:37.821Z]  0:15.07 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/mozilla/meta/MANIFEST.json
[task 2018-05-10T22:15:37.829Z]  0:15.08 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json
[task 2018-05-10T22:15:42.360Z]  0:19.61 WARNING Manifest /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json contains correct tests but file hashes changed; please update
[task 2018-05-10T22:19:47.791Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/lint/wpt.yml:None | Lint process exited with return code 2 (wpt)
[task 2018-05-10T22:19:47.792Z] TEST-UNEXPECTED-ERROR | testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-ellipse-052.html:None | Reference test with a non-existent 'match' relationship reference: 'reference/shape-outside-ellipse-052-ref.html' (NON-EXISTENT-REF)
[task 2018-05-10T22:19:47.792Z] TEST-UNEXPECTED-ERROR | testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/shape-outside-polygon-032.html:None | Reference test with a non-existent 'match' relationship reference: 'reference/shape-outside-polygon-032-ref.html' (NON-EXISTENT-REF)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1754110f8a6
Part 1: Correct PolygonShapeInfo constructor to measure its start and end block extents in margin rect space. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/05f625985b7f
Part 2: Add WPT reftests for many shape-outside shapes that use an element offset from its container. r=jfkthame
Upstream PR was closed without merging
Upstream PR was closed without merging
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/e1754110f8a6
https://hg.mozilla.org/mozilla-central/rev/05f625985b7f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Upstream PR merged
these tests seem to fail in test-verify- new tests should be passing, although it is hard to enforce until we fix all the quirks with test-verify to make it tier1.

you can see the failure here:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=59a49b12b26846302393edfbd20b5e72ef6b1d85&filter-searchStr=tvw%20win
(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #27)
> these tests seem to fail in test-verify- new tests should be passing,
> although it is hard to enforce until we fix all the quirks with test-verify
> to make it tier1.
> 
> you can see the failure here:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=59a49b12b26846302393edfbd20b5e72ef6b1d85&filter-
> searchStr=tvw%20win

Yes, agreed. From browsing the failures, they seem to all be of the type where the edges of the Ahem text glpyhs alias slightly against the background. What we really need is a fuzzy pixel match for WPT reftests, which the spec doesn't define.
Flags: needinfo?(bwerth)
Comment on attachment 8974536 [details]
Bug 1460041 Part 2: Add WPT reftests for many shape-outside shapes that use an element offset from its container.

https://reviewboard.mozilla.org/r/242870/#review252252

So this shouldn't actually have been added this way -- the vendor-imports/mozilla/mozilla-central-reftests/ directory in wpt is imported from our tree -- in layout/reftests/w3c-css/submitted/, which is the master copy.  By not adding them there, this basically causes me to delete the new tests the next time I run the import... unless I fix things by readding them there.
(In reply to David Baron :dbaron: ⌚UTC-7 from comment #29)
 
> So this shouldn't actually have been added this way -- the
> vendor-imports/mozilla/mozilla-central-reftests/ directory in wpt is
> imported from our tree -- in layout/reftests/w3c-css/submitted/, which is
> the master copy.  By not adding them there, this basically causes me to
> delete the new tests the next time I run the import... unless I fix things
> by readding them there.

I'm sorry. I didn't understand that. I have created Bug 1463794 to clean things up.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b80e4986ecfe8e8ec1bdc6aec643b57d4262ffb5
Bug 1460041 Part 2: Add WPT reftests for many shape-outside shapes that use an element offset from its container. r=jfkthame
Backed out part 2 and follow-up for failing new reftests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/88acf3e0794ca8408eec3297e1abb4065cc7a87e

Follow-up only shows test-verify failures at the moment... https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=55bf0e045b5e3ab006e84d7c17d98871714fed94&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

...but shape-outside-circle-056.html  should still be failing: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b80e4986ecfe8e8ec1bdc6aec643b57d4262ffb5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=179852378&repo=mozilla-inbound
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/shapes1/shape-outside-circle-056.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/shapes1/shape-outside-circle-056-ref.html | image comparison, max difference: 255, number of differing pixels: 1344
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/shapes1/shape-outside-ellipse-052.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/shapes1/shape-outside-ellipse-052-ref.html | image comparison, max difference: 255, number of differing pixels: 1344
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/shapes1/shape-outside-polygon-032.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/shapes1/shape-outside-polygon-032-ref.html | image comparison, max difference: 255, number of differing pixels: 28645
Flags: needinfo?(bwerth)
Flags: needinfo?(bwerth) → needinfo?(dbaron)
I don't have time to fix this, but it's blocking import of other people's test changes.

(Either that, or I'll just delete the tests you added next time I import.)
Flags: needinfo?(dbaron) → needinfo?(bwerth)
Flags: needinfo?(bwerth)
You need to log in before you can comment on or make changes to this bug.