Closed Bug 1460041 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Layout: Floats, defect, P2)

Product:
Core
Component:
Layout: Floats
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: jfkthame, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

testcase -- shape-outside:polygon(), shape-margin, float:right
5.03 KB, text/html
Details
Bug 1460041 Part 1: Correct PolygonShapeInfo constructor to measure its start and end block extents in margin rect space.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1460041 Part 2: Add WPT reftests for many shape-outside shapes that use an element offset from its container.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
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
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: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.
Blocks: 1463794
(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.

Attachment

General

Creator:
Created:
Updated:
Size: