Closed Bug 1353164 Opened 8 years ago Closed 5 years ago

dominant-baseline will become inherited in SVG 2 (and CSS Inline 3)

Categories

(Core :: SVG, enhancement, P3)

Product:
Core
Component:
SVG
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox70 --- fixed

People

(Reporter: mpk, Assigned: longsonr)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: site-compat)

Attachments

(3 files, 4 obsolete files)

simple testcase
(deleted), image/svg+xml
Details
Bug 1353164 Part 1 - Remove dominant-baseline values that no longer exist r=heycam
(deleted), text/x-phabricator-request
Details
Bug 1353164 Part 2 - Change dominant-baseline from reset to inherit r=heycam
(deleted), text/x-phabricator-request
Details
Attached image simple testcase (deleted) —
In SVG documents with <g> elements having a text-anchor attribute, their <text> children seem to inherit said attribute. I'd expect the same to happen with the dominant-baseline attribute. In that case no red text would be visible in the attached testcase. Older versions of Opera (v12.16, Presto engine) seem to get it right, while the latest Android browser renders the testcase just as we do.
dominant-baseline isn't an inherited attribute. https://www.w3.org/TR/SVG/text.html#BaselineAlignmentProperties text-anchor is an inherited attribute: https://www.w3.org/TR/SVG/text.html#TextAnchorProperty https://developer.mozilla.org/en-US/docs/Web/CSS/inheritance#Inherited_properties https://developer.mozilla.org/en-US/docs/Web/CSS/inheritance#Non-inherited_properties I guess Opera 12 was wrong.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
(In reply to Robert Longson from comment #1) > dominant-baseline isn't an inherited attribute. > https://www.w3.org/TR/SVG/text.html#BaselineAlignmentProperties Agreed. But we may still want to keep an eye on this topic, because: 1) In the upcoming SVG2 spec, dominant-baseline *is* inherited. (see https://www.w3.org/TR/SVG2/text.html#DominantBaselineProperty) 2) SVG2 is scheduled to become a PR in June 2017 and a TR in July 2017. (see https://www.w3.org/Graphics/SVG/WG/wiki/Roadmap) 3) CSS Inline Layout Module Level 3 (while in the working draft stage) will also support the dominant-baseline property being inherited. (see https://www.w3.org/TR/css-inline-3/#propdef-dominant-baseline) Assuming SVG2 will become the official SVG standard in July, we may want to support dominant-baseline being inheritable in the following release. That would be version 55 (current trunk). I'm not sure how long this would have to ride the nightly train before being released. jwatt, you're listed as an SVG2 editor. A penny for your thoughts on this topic... :-)
Flags: needinfo?(jwatt)
Reopening since SVG2 compliance will depend on this.
Blocks: svg2
URL: https://svgwg.org/svg2-draft/text.htm...
Status: RESOLVED → REOPENED
Flags: needinfo?(jwatt)
Resolution: INVALID → ---
Summary: <text> elements don't inherit the dominant-baseline attribute from their <g> parents → dominant-baseline will become inherited in SVG 2 (and CSS Inline 3)
Blocks: css-inline
Attached patch part 1 - gecko (obsolete) (deleted) — — Splinter Review
Assignee: nobody → longsonr
Attachment #8881115 - Flags: review?(cam)
Attached patch part 2 - servo (obsolete) (deleted) — — Splinter Review
Attachment #8881137 - Flags: review?(cam)
Do we still need the for look here Cameron? https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#9589
Flags: needinfo?(cam)
sorry make that for loop, not for look.
Sorry for the review delay, it'll have to be next week before I take a look.
(In reply to Robert Longson from comment #6) > Do we still need the for look here Cameron? > https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame. > cpp#9589 Yeah, looks like we don't need that any more.
Flags: needinfo?(cam)
Comment on attachment 8881115 [details] [diff] [review] part 1 - gecko Review of attachment 8881115 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that, and with the loop removed from nsIFrame::VerticalAlignEnum. ::: layout/style/nsRuleNode.cpp @@ +9669,5 @@ > + // dominant-baseline: enum, inherit, initial > + SetValue(*aRuleData->ValueForDominantBaseline(), > + svg->mDominantBaseline, > + conditions, > + SETVAL_ENUMERATED | SETVAL_UNSET_INITIAL, This should be SETVAL_UNSET_INHERIT, now that it's an inherited property. ::: layout/style/nsStyleStruct.cpp @@ -1222,5 @@ > } > > - if (mDominantBaseline != aNewData.mDominantBaseline) { > - // XXXjwatt: why NS_STYLE_HINT_REFLOW? Isn't that excessive? > - hint |= NS_STYLE_HINT_REFLOW; I'm not really sure what change hints we need for dominant-baseline, but I would expect it needs to be the same as vertical-align (since we hack it to be implemented in terms of vertical-align values). So I would prefer to keep returning NS_STYLE_HINT_REFLOW for dominant-baseline changes.
Attachment #8881115 - Flags: review?(cam) → review+
Attachment #8881137 - Flags: review?(cam) → review+
Priority: -- → P3
Is this ready to land?
Flags: needinfo?(longsonr)
Flags: needinfo?(cam)
It was, it may have bitrotted now, but I've no idea how to land it because it seemed to need simultaneous changes on gecko and servo. I can unbit rot it if necessary but I suspect I still wouldn't know how to land it.
Flags: needinfo?(longsonr)
Simultaneously landing on https://github.com/servo/servo/ and mozilla-central is no longer needed. You can just make changes to the files directly under servo/ in the same patch as the other m-c changes. Robert, if you have time to unbitrot the patch that would be great, thanks!
Flags: needinfo?(cam) → needinfo?(longsonr)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:longsonr, could you have a look please?

Flags: needinfo?(longsonr)
Attached patch Part 1 - remove values that no longer exist (obsolete) (deleted) — — Splinter Review
Attachment #8881115 - Attachment is obsolete: true
Attachment #8881137 - Attachment is obsolete: true
Flags: needinfo?(longsonr)
Attached patch 1353164-1.txt (obsolete) (deleted) — — Splinter Review
Attachment #9073352 - Attachment is obsolete: true

For reference new spec is here: https://www.w3.org/TR/css-inline-3/#propdef-dominant-baseline

Part 2 will make it inherited.

Attachment #9073386 - Attachment is obsolete: true
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a211bcf50767 Part 1 - Remove dominant-baseline values that no longer exist r=heycam
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17823 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Keywords: leave-open

Could you write an intent email for this?

Keywords: site-compat
Keywords: leave-open
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c8a0f41b86d Part 2 - Change dominant-baseline from reset to inherit r=heycam

Backed out changeset 5c8a0f41b86d (Bug 1353164) for test_css-properties-db.js failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&searchStr=xpcshell%2Ctests&fromchange=17ad24a685ef790a9e022013763c73022d4f5714&tochange=233aa9249d9d3a398d7ada1e1000100f6db000d6&selectedJob=257384949

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/233aa9249d9d3a398d7ada1e1000100f6db000d6

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257384949&repo=mozilla-inbound&lineNumber=4984

13:22:45 INFO - TEST-START | devtools/server/tests/unit/test_objectgrips-21.js
13:22:49 INFO - TEST-PASS | devtools/server/tests/unit/test_objectgrips-21.js | took 4001ms
13:22:49 INFO - TEST-START | devtools/shared/tests/unit/test_css-properties-db.js
13:22:50 WARNING - TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_css-properties-db.js | xpcshell return code: 0
13:22:50 INFO - TEST-INFO took 479ms

Flags: needinfo?(longsonr)

I fixed this some time ago via https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e83b3461c2814410e71fe36bf63fea7e15d5e2 you'll need to reland the first fix.

Flags: needinfo?(longsonr) → needinfo?(btara)
Pushed by btara@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea04f98d752a Part 2 - Change dominant-baseline from reset to inherit r=heycam
Flags: needinfo?(btara)

https://hg.mozilla.org/mozilla-central/rev/b2e83b3461c2
https://hg.mozilla.org/mozilla-central/rev/ea04f98d752a

Status: REOPENED → RESOLVED
Closed: 8 years ago → 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: