Closed Bug 1111463 Opened 5 years ago Closed 5 years ago

Update default ruby styles in reftests and ua style sheet

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: xidorn, Assigned: xidorn)

References

()

Details

Attachments

(3 files, 3 obsolete files)

The default stylesheet has been updated, and might be further updated soon. We need to update our stylesheets in tests and ua.css to meet the standard behavior.
Attached patch patch (obsolete) — Splinter Review
Attachment #8538269 - Flags: review?(dbaron)
Comment on attachment 8538269 [details] [diff] [review]
patch

>-[pseudo] {
>-  font-size: inherit !important;
>-  line-height: inherit !important;
> }
>-[pseudo] > rt {
>-  font-size: 50%;
>+
>+rt[pseudo] {
>+  font-size: 200%;
> }

Why is this selecting on an attribute called "pseudo"?
Flags: needinfo?(quanxunzhen)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #2)
> Comment on attachment 8538269 [details] [diff] [review]
> patch
> 
> >-[pseudo] {
> >-  font-size: inherit !important;
> >-  line-height: inherit !important;
> > }
> >-[pseudo] > rt {
> >-  font-size: 50%;
> >+
> >+rt[pseudo] {
> >+  font-size: 200%;
> > }
> 
> Why is this selecting on an attribute called "pseudo"?

I use this attribute to mark the counterpart of pseudo boxes in the test page. I think I really should use "rtc[pseudo] > rt[pseudo]" instead of only "rt[pseudo]" here. It is for intra-annotation whitespace which is wrapped in an anonymous rtc. It inherits the styles from <ruby>.
Flags: needinfo?(quanxunzhen)
Comment on attachment 8538269 [details] [diff] [review]
patch

Oops, I was misreading which part of the patch was patching ua.css.

So where does the spec say that these style should apply to the anonymous boxes?
Flags: needinfo?(quanxunzhen)
No, I don't think spec currently says anything about the style of anonymous boxes. But we should really apply some styles to the boxes, or they will affect line height in an unexcepted way by default.
Flags: needinfo?(quanxunzhen)
s/unexcepted/unexpected/

And given that there are default styles on rt and rtc, I think it's fine to also have the same style on the corresponding anonymous boxes. Authors should expect this behavior.
Comment on attachment 8538269 [details] [diff] [review]
patch

So if the spec doesn't say that the default style should have the ruby text be smaller, then I don't think we should implement such a behavior.  If you think the spec should change, bring that up on www-style (and be patient about it).

Is the reason that you're specifying 50% on both the ruby text and the ruby text container that element children of the ruby text container don't inherit from the anonymous boxes, but text frames do?  If so, please explain that in a comment.

If that's the case, how does this handle the case of an inline element inside of an anonymous ruby text *and* an anonymous ruby text container?  Or do the anonymous box generation rules ensure that you never have an anonymous ruby text inside an anonymous ruby text container?

Finally, there aren't any styles for HTML ruby elements checked in that you can compare these to.  It seems like the default styles that *are* in the spec should be checked in to the tree, inside an "@supports (display:ruby)", which would allow useful comparison between the anonymous box styles and the HTML styles during code reviews.
Attachment #8538269 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #7)
> Comment on attachment 8538269 [details] [diff] [review]
> patch
> 
> So if the spec doesn't say that the default style should have the ruby text
> be smaller, then I don't think we should implement such a behavior.  If you
> think the spec should change, bring that up on www-style (and be patient
> about it).

Ruby text should be smaller if it is not wrapped by a non-pseudo rtc. The default stylesheet has

  rt, rtc { font-size: 50%; }
  rtc > rt { font-size: inherit; }

I guess I should add a rule like "rtc > *|*::-moz-ruby-text { font-size: inherit; }", should I?

> Is the reason that you're specifying 50% on both the ruby text and the ruby
> text container that element children of the ruby text container don't
> inherit from the anonymous boxes, but text frames do?  If so, please explain
> that in a comment.

I don't think text frames inherit either. The font-size are placed there only for the line-height calculation.

> If that's the case, how does this handle the case of an inline element
> inside of an anonymous ruby text *and* an anonymous ruby text container?  Or
> do the anonymous box generation rules ensure that you never have an
> anonymous ruby text inside an anonymous ruby text container?

The anonymous box generation rules ensure that with one exception, which is the intra-annotation whitespace.
Attachment #8538269 - Attachment is obsolete: true
Attachment #8540431 - Flags: review?(dbaron)
Attached patch patch 3 - Fix reftests (obsolete) — Splinter Review
Attachment #8540432 - Flags: review?(dbaron)
Actually, I'm worry about the interaction between this bug and bug 1055665. There might be further reftest fix in either bug depending on which lands first.
Attachment #8540429 - Flags: review?(dbaron) → review+
Comment on attachment 8540431 [details] [diff] [review]
patch 2 - Update html.css and ua.css

>+@supports(display: ruby) {

Please put a space between "@supports" and "(".

(And please fix the same in ua.css.)

>diff --git a/layout/style/ua.css b/layout/style/ua.css
>   *|*::-moz-ruby-text {
>     display: ruby-text;
>+    /* font-size and line-height here are meant to correct the line
>+     * height calculation, and won't be inherited by any descendent */
>+    font-size: 50%; line-height: 1;
>+  }
>+  rtc > *|*::-moz-ruby-text {
>+    font-size: inherit;
>   }
>   *|*::-moz-ruby-base-container {
>     display: ruby-base-container;
>   }
>   *|*::-moz-ruby-text-container {
>     display: ruby-text-container;
>+    /* font-size and line-height here are meant to correct the line
>+     * height calculation, and won't be inherited by any descendent */
>+    font-size: 50%; line-height: 1;
>   }

I still don't think you should make these changes if the spec doesn't call for them.

r=dbaron with the ua.css changes removed, though
Attachment #8540431 - Flags: review?(dbaron) → review+
Attachment #8540432 - Flags: review?(dbaron) → review+
About the line-height of pseudo rtc, the spec says:

 Each ruby annotation container is sized and positioned to contain exactly the full height of its ruby annotations. [1]

I do not fully agree with this spec, because I see no reason to disallow author from specifying line-height to rtc, except that other UAs do so currently. And to fully meet this requirement, we need to hack a little in the line layout. (This could be in an independent bug anyway.)

But what I do agree is, the container shouldn't be larger than its rt children by default, especially when the container is pseudo, and author has no way to control over.

Given this, I'm thinking about changing line-height of ::-moz-ruby-text-container to zero directly.

In addition, given fantasai's reply to my email [2], it might also make sense to set line-height of ::-moz-ruby-text to zero, but make "rtc > *|*::-moz-ruby-text" inherits the line-height, because a pseudo ruby text must be an intra-annotation whitespace if it is not inside a non-pseudo ruby text container.

[1]: http://dev.w3.org/csswg/css-ruby/#ruby-layout
[2]: http://lists.w3.org/Archives/Public/www-style/2014Nov/0419.html
Change line-height to zero so that it won't add any extra spacing to the ruby text. Note that this works correctly only with patches for bug 1055665 applied.
Attachment #8540431 - Attachment is obsolete: true
Attachment #8541071 - Flags: review?(dbaron)
Attachment #8540432 - Attachment is obsolete: true
Attachment #8541072 - Flags: review?(dbaron)
Why are you revising already-reviewed patches to add a separate bug fix rather than putting that separate bug fix in its own bug?
Flags: needinfo?(quanxunzhen)
I don't think this change fixes the bug I mentioned in comment 14. That bug would be about suppressing any spacing of ruby text container, even it is specified by the author. This change only suppresses the spacing added by the ua stylesheet by default.
Flags: needinfo?(quanxunzhen)
The line height stuff is somewhat complicated and I'd rather discuss it in a separate bug.
I filed bug 1115249 for the spec problem. But I still think we can fix the simple default behavior in this bug.
Comment on attachment 8541071 [details] [diff] [review]
patch 2 - Update html.css and ua.css

>   *|*::-moz-ruby-text {
>     display: ruby-text;
>+    /* A pseudo ruby text which is not inside a ruby text container
>+     * must be a inter-annotation whitespace. Suppress its line-height
>+     * so that it won't affect the height of the container. */
>+    line-height: 0;
>+  }
>+  rtc > *|*::-moz-ruby-text {
>+    line-height: inherit;
>   }

First of all, your comment isn't correct.  You might have a pseudo-ruby text that matches the first rule but not the second resulting from
<span style="display:ruby-text-container">text</span>.

So I don't think you want these changes, and I think you should leave them until you're implementing the line height rules for ruby.

>   *|*::-moz-ruby-text-container {
>     display: ruby-text-container;
>+    /* Suppress the line-height of pseudo ruby text container, so
>+     * that it will not be taller than its ruby text children. */
>+    line-height: 0;
>   }

I don't think the spec supports this, so you don't want this change either.  (But it should be less of an issue once you implement the spec's rules on line height calculation and ruby.)

r=dbaron with these two changes dropped
Attachment #8541071 - Flags: review?(dbaron) → review+
Comment on attachment 8541072 [details] [diff] [review]
patch 3 - Fix reftests

I think my comments on patch 2 lead to this patch reverting to the previously-reviewed patch.
Attachment #8541072 - Flags: review?(dbaron) → review-
OK, so let's back to use the previously reviewed one, and implement bug 1115249.
What about the rule:

 rbc { display: ruby-base-container }

which isn't in html.css (and is still in layout/reftests/css-ruby/common.css).  Shouldn't that be in html.css too?  (Followup bug, perhaps... or maybe even just a followup patch here?)
Flags: needinfo?(quanxunzhen)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #27)
> What about the rule:
> 
>  rbc { display: ruby-base-container }
> 
> which isn't in html.css (and is still in
> layout/reftests/css-ruby/common.css).  Shouldn't that be in html.css too? 
> (Followup bug, perhaps... or maybe even just a followup patch here?)

I don't think it should be in html.css, because we don't have rbc tag in html. (I believe we don't parse that tag, either)

See http://dev.w3.org/csswg/css-ruby/#valdef-display-ruby-base-container , only xhtml has <rbc> element, html doesn't.
Flags: needinfo?(quanxunzhen)
You need to log in before you can comment on or make changes to this bug.