Closed Bug 1353164 Opened 3 years ago Closed 4 months ago

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

Categories

(Core :: SVG, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox55 --- wontfix
firefox70 --- fixed

People

(Reporter: mpk, Assigned: longsonr)

References

(Blocks 2 open bugs, )

Details

(Keywords: site-compat)

Attachments

(3 files, 4 obsolete files)

Attached image simple testcase
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: 3 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
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) — Splinter Review
Assignee: nobody → longsonr
Attachment #8881115 - Flags: review?(cam)
Attached patch part 2 - servo (obsolete) — 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)
Attachment #8881115 - Attachment is obsolete: true
Attachment #8881137 - Attachment is obsolete: true
Flags: needinfo?(longsonr)
Attached patch 1353164-1.txt (obsolete) — 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
Upstream PR merged

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)
Status: REOPENED → RESOLVED
Closed: 3 years ago4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.