Closed Bug 1135306 Opened 5 years ago Closed 5 years ago

ruby-base and ruby-text have an effect even outside ruby

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: emk, Assigned: xidorn)

References

()

Details

Attachments

(3 files, 2 obsolete files)

Attached file rt-outside-ruby.html (obsolete) —
I'm not sure about the correct behavior, but filing anyway.

Steps to reproduce:
Open the attached testcase.

Actual result:
1. == 2. != 3. != 4.

Expected result (?):
1. != 2. == 3. == 4.

ruby-base and ruby-text outside ruby have no effect on other browsers (at least Chrome and IE 11)

Is this expected? Which is the correct behavior per spec? (I assume modern specs will not leave errors undefined).
Attached file rt-outside-ruby.html
Testcase updated.
Actual:
1. == 2. == 3. != 3. != 4.

Expected (?):
1. == 2. != 3. == 4. == 5.
Attachment #8567408 - Attachment is obsolete: true
> 1. == 2. == 3. != 3. != 4.
Sorry, 1. == 2. == 3. != 4. != 5.
I suppose that it's something related to tokenization of HTML instead of CSS Ruby.

The CSS Ruby spec does require create anonymous boxes for improperly-contained boxes [1], but the HTML5 spec requires ignore the <rt>s when they are not in a <ruby> [2]. However, the WHATWG HTML5 spec doesn't include anything about <rb> and <rtc>, but I suppose it would say the same thing as <rt>.

This difference is similar to the table layout. I guess we need to change our HTML tokenizer, but not CSS frame constructor.

[1] http://dev.w3.org/csswg/css-ruby-1/#box-fixup
[2] https://html.spec.whatwg.org/multipage/semantics.html#the-rt-element
Version: 37 Branch → Trunk
Blocks: ruby
No longer blocks: css-ruby
Attached file pure CSS testcase
How about this? HTML tokenizer is obviously irrelevant.

Nightly:
1. != 2. != 3.

IE and Nightly before bug 1134206:
1. != 2. == 3.

Chomre (because Chrome does not support CSS ruby at all):
1. == 2. == 3.
For the display values like this testcase, our behavior is definitely correct. See the CSS Ruby link I provided in comment 3.

I mentioned that it is something similar to table, because a <td> outside <table> will be ignored, but an element with display: table-cell will generate a table.
Component: Layout → HTML: Parser
Summary: ruby-base and ruby-text have an effect even outside ruby → <rb>, <rt>, <rtc>, <rp> tags should be ignored when they are not contained by proper parent
Blocks: 664104
What does "ignored" mean?  What's the expected output DOM?
I thought that the children of the element would take its place, just like what happens to <td> outside <table>. The spec says:
"An rt element that is not a child of a ruby element represents the same thing as its children."

But it seems I was wrong. I searched "represents its children" in the spec, and find this expression is used for <div> and <span>, which means it shouldn't affect the output DOM.

Now, it is a conflict between HTML5 spec and CSS Ruby spec.

In CSS Ruby spec, the default display of ruby tags are defined to corresponding display values [1], and the spec also requires fixing up the ruby structure even if a ruby element is not properly contained [2].

However, in HTML5 spec (both WHATWG and W3C), it says an improperly contained inner ruby element should represent its children instead of anything else. [3]

The easiest way to fix this would be changing the default stylesheet in CSS Ruby spec I guess.

[1] http://dev.w3.org/csswg/css-ruby-1/#default-ua-ruby
[2] http://dev.w3.org/csswg/css-ruby-1/#anon-gen-anon-ruby
[3] https://html.spec.whatwg.org/multipage/semantics.html#the-rt-element
Component: HTML: Parser → Layout
Summary: <rb>, <rt>, <rtc>, <rp> tags should be ignored when they are not contained by proper parent → ruby-base and ruby-text have an effect even outside ruby
Attached patch patch (obsolete) — Splinter Review
This patch changes the html.css to match what HTML5 spec requires. Since some of the reftests are designed to test the box-fixup of CSS Ruby, there are some styles added to fix those reftests.
Assignee: nobody → quanxunzhen
Attachment #8573529 - Flags: review?(dholbert)
Attached patch patchSplinter Review
Attachment #8573529 - Attachment is obsolete: true
Attachment #8573529 - Flags: review?(dholbert)
Attachment #8573541 - Flags: review?(dholbert)
Comment on attachment 8573541 [details] [diff] [review]
patch

>diff --git a/layout/reftests/css-ruby/box-generation-4-ref.html b/layout/reftests/css-ruby/box-generation-4-ref.html
>--- a/layout/reftests/css-ruby/box-generation-4-ref.html
>+++ b/layout/reftests/css-ruby/box-generation-4-ref.html
>@@ -34,16 +34,16 @@
>-      </span><ruby><rbc><rb>u</rb></rbc><rbc><rbc><rb><span> </span></rb></rbc
>+      </span><ruby><rbc><rb>u</rb></rbc><rbc><rb><span> </span></rb></rbc

This part (removing a double-nested <rbc>) seems unrelated to the rest of the patch here. (and worth fixing independently of the rest of the stuff here)

Might be better to land that on its own, to make the rest of this bug's patch clearer -- if you like, rs=me on pushing that as a standalone patch, as something like
Bug 1088489 followup: remove accidentally nested <rbc> from box-generation-4-ref.html rs=dholbert

(Or, if you prefer, it's fine to leave it included here too.)
Is this introducing elements into the style sheet that aren't defined in the HTML spec?

Also, these selectors seem rather more expensive.  Is having this error handling really worth that performance cost, or should we try to get the specs made more consistent?
Comment on attachment 8573541 [details] [diff] [review]
patch

Review of attachment 8573541 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following addressed:

::: layout/reftests/css-ruby/justification-2-ref.html
@@ +14,5 @@
>      }
>    </style>
>  </head>
>  <body>
> +  <div class="ruby"><rb>仮</rb><rb>名</rb><rt>が</rt><rt>な</rt></div>

The |class="ruby"| tweak you're making to all of these testcases is misleading & easy to misunderstand.

In particular -- it pretty strongly suggests that the "<div>" gets "display:ruby".  But, that is *not* what happens. In reality, it activates an alternate set of styles in common.css, which emulate the html.css rules, to *work around HTML5's requirement* of having a <ruby> parent.

So, to clear that up -- I'd suggest renaming this class to something clearer that's harder to interpret as 'display:ruby'. Some possibilities:
  "honor-ruby-tags"
  "honor-ruby-parts"

Also, the definition of this class in common.css needs a brief explanation for why this class exists (mentioning that the UA stylesheet *only* treats <rb> as "display:ruby-base" inside of a <ruby> tag -- but in our testcases, we want to be able to use <rb> more broadly than that, as a "display:ruby-base" thing even outside of <ruby>.)

::: layout/reftests/w3c-css/submitted/ruby/support/ruby-tags.css
@@ +1,3 @@
> +ruby { display: ruby; }
> +rb { display: ruby-base; }
> +rt { display: ruby-text; }

Why do we bother with "ruby { display:ruby }" here? Is the goal to allow these tests to be run, *without needing the UA-stylesheet rules* in html.css? If so: that makes sense, but it's worth documenting that in a comment.

Also: this file needs to mention that (& briefly explain why) it's non-conformant with HTML5 on e.g. naked <rb> rendering as "ruby-base" etc.  Otherwise, it's likely to cause surprise/confusion when someone takes code from one of your testcases & expects it to work in another context, leaning on their UA stylesheet instead of this ruby-tags.css file.  (There may be no avoiding that sort of surprise, but a comment will at least give them a warning & a chance of discovering the problem, when someone runs into that.)

::: layout/style/html.css
@@ +848,1 @@
>      unicode-bidi: isolate;

This should be "-moz-isolate" -- we only support that prefixed value[1] -- so the rule you're updating here actually has no effect right now, I think.

Feel free to fix that (add the prefix) as part of this patch, or [maybe slightly better] in a separate patch on this bug (rs=me on that patch)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp?rev=6fb93b485e4c&mark=1783-1783#1779
Attachment #8573541 - Flags: review?(dholbert) → review+
(Sorry, just saw dbaron's comment:)

(In reply to David Baron [:dbaron] (UTC-8) from comment #11)
> Is this introducing elements into the style sheet that aren't defined in the
> HTML spec?

The HTML5 spec doesn't include <rtc> or <rbc>, but there's a bug open on adding them (and it superficially seems like it's going to happen): https://www.w3.org/Bugs/Public/show_bug.cgi?id=26189

We already include support for <rtc>/<rbc> tags in html.css. Given that we already support these tags, this patch takes the logical step of extending the spec's "<ruby><rt>" nesting requirement to also allow "<ruby><rtc><rt>".

> Also, these selectors seem rather more expensive.  Is having this error
> handling really worth that performance cost, or should we try to get the
> specs made more consistent?

I was a bit worried about that, too, though I clearly don't have as good a handle on selector perf as dbaron. (sorry for not bringing that up)  I'll defer to dbaron on this being worth-considering.
(In reply to Daniel Holbert [:dholbert] from comment #13)
> The HTML5 spec doesn't include <rtc> or <rbc>, but there's a bug open on
> adding them (and it superficially seems like it's going to happen):
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=26189

In particular https://www.w3.org/Bugs/Public/show_bug.cgi?id=26189#c4 suggests the spec will end up being changed (though I'm a bit curious about the backstory there).

Also, I suppose that bug only covers <rtc>, *not* <rbc>. xidorn, do you know what the story is with <rbc>?  I can't find a spec bug on it, right away.
Flags: needinfo?(quanxunzhen)
Long story to short: battle between W3C and WHATWG.
W3C HTML5 and HTML 5.1 Nightly adopted <rb>/<rtc> [1][2][3][4], but WHATWG people hate them and refused to merge them to the HTML Living Standard.
<rbc> is a vestige of XHTML 1.1 ruby.
Xidorn will explain more.

[1] http://www.w3.org/TR/html5/text-level-semantics.html#the-rb-element
[2] http://www.w3.org/TR/html5/text-level-semantics.html#the-rtc-element
[3] http://www.w3.org/html/wg/drafts/html/master/semantics.html#the-rb-element
[4] http://www.w3.org/html/wg/drafts/html/master/semantics.html#the-rtc-element
I guess we probably should defer this patch, I just found that the HTML5 specs conflict themselves.

Although the spec says rp/rt/rtc tags are only effective when they are in a proper structure [1][2], the user agent stylesheet section indicates that those styles are applied unconditionally [3][4]. Which means, either the HTML5 specs conflict themselves, or they conflict with the box fixup algorithm in CSS Ruby.

I have no idea what should happen then.

[1] https://html.spec.whatwg.org/multipage/semantics.html#the-rt-element
[2] http://www.w3.org/TR/html5/text-level-semantics.html#the-rb-element
[3] https://html.spec.whatwg.org/multipage/rendering.html#phrasing-content-3
[4] http://www.w3.org/TR/html5/rendering.html#phrasing-content-0
Comment on attachment 8573541 [details] [diff] [review]
patch

[Relaxing my review+ to feedback+, then, given that we probably shouldn't take this patch at this point.]

We should still fix the extra nested <rtc> mentioned in comment 10, and fix the "isolate" --> "-moz-isolate" issue mentioned at the end of comment 12. (and we need to add tests that depend on the "-moz-isolate" style -- that's presumably untested now, given that the current "isolate" styling should be rejected & would break any tests on that behavior)
Attachment #8573541 - Flags: review+ → feedback+
(In reply to Daniel Holbert [:dholbert] from comment #14)
> (In reply to Daniel Holbert [:dholbert] from comment #13)
> > The HTML5 spec doesn't include <rtc> or <rbc>, but there's a bug open on
> > adding them (and it superficially seems like it's going to happen):
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26189
> 
> In particular https://www.w3.org/Bugs/Public/show_bug.cgi?id=26189#c4
> suggests the spec will end up being changed (though I'm a bit curious about
> the backstory there).

emk's right, the WHATWG people, especially hixie really hates <rb> and <rtc>, and thinks it is overengineering. I'm not clear about the backstory happened between W3C and WHATWG, but I see the result as well as hixie's comments on our bugzilla [1] and the mailing list [2].

> Also, I suppose that bug only covers <rtc>, *not* <rbc>. xidorn, do you know
> what the story is with <rbc>?  I can't find a spec bug on it, right away.

<rbc> is not a HTML tag listed in either spec, and we do not have <rbc> in our HTML parser, either. It is a XHTML tag. In the current html.css, there is no <rbc> rule, either. I'm fine to remove <rbc> related rules in this patch.

I added it in this patch because the default stylesheet in CSS Ruby has it [3], and interestingly, the stylesheet of W3C HTML5 also includes it [4]. I'm not sure whether we should include it. I did hear some complain that the result is strongly undesired if this XHTML tag is used. [5]

Probably it should be discussed in another bug, though.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=33339#c110
[2] https://groups.google.com/d/msg/mozilla.dev.platform/VqSAwVuskm8/qqlT3jZVI9EJ
[3] http://dev.w3.org/csswg/css-ruby-1/#default-ua-ruby
[4] http://www.w3.org/TR/html5/rendering.html#phrasing-content-0
[5] http://momdo.hatenablog.jp/entry/20150309/1425904075
Flags: needinfo?(quanxunzhen)
I don't see any conflict in the spec based on the comments above.  The quote at the start of comment 7 is describing the semantics of the language, and is unrelated to requirements on implementations.  (I think the second paragraph of comment 7 is wrong.)  If implementations are required to do something, that requirement will be stated.

(CSS specs aren't always that clear, but in general, that's probably how you *ought* to interpret specs.  In the case of CSS, some spec authors will forget to write the implementation requirements explicitly.  See http://ln.hixie.ch/?start=1140242962&count=1 for more.)
Thanks for the link. So that means we can just keep it as-is and close this bug with WONTFIX, right?
Given that the "represents" statement is just a semantics statement, which doesn't make sense to the rendering behavior, and the default stylesheet in the HTML5 spec clearly support our current behavior, I think we should close it with INVALID.

If the spec changes in the future, we may reopen it to meet that new requirement, but currently it is not a bug.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.