Closed Bug 1353164 Opened 3 years ago Closed 4 months ago

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


(Core :: SVG, enhancement, P3)




Tracking Status
firefox55 --- wontfix
firefox70 --- fixed


(Reporter: mpk, Assigned: longsonr)


(Blocks 2 open bugs, )


(Keywords: site-compat)


(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.

text-anchor is an inherited attribute:

I guess Opera 12 was wrong.
Closed: 3 years ago
Resolution: --- → INVALID
(In reply to Robert Longson from comment #1)
> dominant-baseline isn't an inherited attribute.

Agreed. But we may still want to keep an eye on this topic, because:

1) In the upcoming SVG2 spec, dominant-baseline *is* inherited.

2) SVG2 is scheduled to become a PR in June 2017 and a TR in July 2017.

3) CSS Inline Layout Module Level 3 (while in the working draft stage)
   will also support the dominant-baseline property being inherited.

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
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?
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?
> 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,

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 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:

Part 2 will make it inherited.

Attachment #9073386 - Attachment is obsolete: true
Pushed by
Part 1 - Remove dominant-baseline values that no longer exist r=heycam
Created web-platform-tests PR 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
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:

Backout link:

Failure log:

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 you'll need to reland the first fix.

Flags: needinfo?(longsonr) → needinfo?(btara)
Pushed by
Part 2 - Change dominant-baseline from reset to inherit r=heycam
Flags: needinfo?(btara)
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.