1165538 - font-size of ruby-text shouldn't be limited by "Minimum font size" setting (font.minimum-size.*)

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

1. See a ruby annotation example
2. Set "Minimum font size" large enough value

Actual result:

Both ruby text and ruby base are rendered with same font size.

Expected result:

The ruby base should be rendered as the minimum font size but ruby text should be smaller than it.


HTML5 spec suggests rt's font size is 50%.

http://www.w3.org/TR/html5/rendering.html#phrasing-content-0
> rt   {
>     display: ruby-text;
>     white-space: nowrap;
>     font-size: 50%;
>     font-variant-east-asian: ruby;
>     text-emphasis: none;
> }

So, I think that ruby text's minimum font size should be half of specified minimum font size.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

If we really want to solve this problem, we will need to do the similar thing as for bug 1055672.

The minimum font size is taken into consider when computing StyleFont and StyleText, and if we want to ignore minimum font size for ruby text, we will need to make StyleFont depends on StyleDisplay. Since the "display" property does not actually depend on font size, conceptually it is fine. But moving a bunch of properties around style structures seems to be a pain.

Anyway, this bug adds justification to bug 1139283.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> The minimum font size is taken into consider when computing StyleFont and StyleText

Wow, sounds complicated... I was thinking that when referring the style, we could adjust with the display value. (and also at returning the computed value)

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Not very complicated, just need to resolve some dependency problem in our code. In our current codebase, nsStyleDisplay depends on nsStyleFont because it includes some properties which accepts length values (e.g. transform, scroll-snap), those values need nsStyleFont to compute.

Once bug 1139283 gets fixed, this bug is easy.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

It seems I was wrong. Fixing bug 1139283 won't make this bug trivial. (Hence remove the dependency)

Although having nsStyleFont depend on nsStyleDisplay could probably work for most common cases, but it isn't perfect, and may produce weird result in some cases, like nested tags inside <rt> which specifies a different font info (not necessary font-size, could also be font-weight etc.)

One possible way which could completely fix this is to add an internal-only inherited CSS property, e.g. -moz-ignore-min-font-size, and compute font size based on that, instead of display.

Then, sadly, the additional justification for bug 1139283 is lost again :(

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1165538 part 1 - Use delegated constructor for nsStyleFont to simplify code and remove the need of nsStyleFont::Init.

Bug 1165538 part 1 - Use delegated constructor for nsStyleFont to simplify code and remove the need of nsStyleFont::Init.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1165538 part 2 - Add -moz-min-font-size-ratio internal property.

Bug 1165538 part 2 - Add -moz-min-font-size-ratio internal property.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1165538 part 3 - Apply -moz-min-font-size-ratio to rt elements and add test.

Bug 1165538 part 3 - Apply -moz-min-font-size-ratio to rt elements and add test.

Comment 8

[Cameron McCormack (:heycam)]

This means that authors who create ruby with display:ruby etc. and without using the standard HTML ruby elements won't get this minimum font size behaviour.  Is that OK?

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> Although having nsStyleFont depend on nsStyleDisplay could probably work for
> most common cases, but it isn't perfect, and may produce weird result in
> some cases, like nested tags inside <rt> which specifies a different font
> info (not necessary font-size, could also be font-weight etc.)

I didn't understand this, and why depending on display for the minimum font size calculations couldn't work.  Can you give an example?

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8668803 [details]
MozReview Request: Bug 1165538 part 1 - Use delegated constructor for nsStyleFont to simplify code and remove the need of nsStyleFont::Init.

https://reviewboard.mozilla.org/r/21049/#review19071

::: layout/style/nsStyleStruct.h:118
(Diff revision 1)
> +  static already_AddRefed<nsIAtom> LanguageAtom(nsPresContext* aPresContext);

Nit: I think "Atom" in the function name doesn't add anything.  Maybe just call it "GetLanguage" or "GetContentLanguage".

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) from comment #8)
> This means that authors who create ruby with display:ruby etc. and without
> using the standard HTML ruby elements won't get this minimum font size
> behaviour.  Is that OK?

And also, if authors explicitly set <rt> to display:inline, <rt> will still have a different minimum font size. I guess those should be very common.

(In reply to Cameron McCormack (:heycam) from comment #9)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> > Although having nsStyleFont depend on nsStyleDisplay could probably work for
> > most common cases, but it isn't perfect, and may produce weird result in
> > some cases, like nested tags inside <rt> which specifies a different font
> > info (not necessary font-size, could also be font-weight etc.)
> 
> I didn't understand this, and why depending on display for the minimum font
> size calculations couldn't work.  Can you give an example?

What I thought was to take display value into account when getting the minimum font size, and not to add any field in nsStyleFont. That won't work because tags inside <rt> may have different StyleFont instance, and thus won't get the fixed size.

If we persist that in StyleFont and let it be inherited, it would probably work. The remaining question is, whether it is a good idea to make an inherit style struct depend on a reset style struct.

If we do not make it depend in nsRuleNode, but instead do it in the fixup phase in nsStyleContext... it probably works. The only issue is that we may need to create one unique StyleFont for each <rt>/<rtc> element.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> (In reply to Cameron McCormack (:heycam) from comment #8)
> > This means that authors who create ruby with display:ruby etc. and without
> > using the standard HTML ruby elements won't get this minimum font size
> > behaviour.  Is that OK?
> 
> And also, if authors explicitly set <rt> to display:inline, <rt> will still
> have a different minimum font size. I guess those should be very common.

I meant, those should *not* be very common...

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12)
> > And also, if authors explicitly set <rt> to display:inline, <rt> will still
> > have a different minimum font size. I guess those should be very common.
> 
> I meant, those should *not* be very common...

I hope authors don't try to use <rt> to subvert the user's minimum font size settings...

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> What I thought was to take display value into account when getting the
> minimum font size, and not to add any field in nsStyleFont. That won't work
> because tags inside <rt> may have different StyleFont instance, and thus
> won't get the fixed size.

I see.

> If we persist that in StyleFont and let it be inherited, it would probably
> work. The remaining question is, whether it is a good idea to make an
> inherit style struct depend on a reset style struct.

I am not certain, but I think we would never be able to cache a Font struct in the rule tree if we did this.

> If we do not make it depend in nsRuleNode, but instead do it in the fixup
> phase in nsStyleContext... it probably works. The only issue is that we may
> need to create one unique StyleFont for each <rt>/<rtc> element.

Yeah. :(

I wonder if we could have a mechanism to cache these fixed-up structs on the rule node, so we didn't have to one copy of them per element?

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) from comment #13)
> I wonder if we could have a mechanism to cache these fixed-up structs on the
> rule node, so we didn't have to one copy of them per element?

"Don't try any new ownership schemes, please." [1] - dbaron

:)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1055667#c41

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> "Don't try any new ownership schemes, please." [1] - dbaron

OK. :-)  Well, I don't think it would be that bad, but maybe not something to try just for this bug.  (Depends what David meant exactly, of course -- perhaps he meant that using the dependent bit to additionally mean that we could be referencing a statically allocated empty struct would be too far different from the existing meaning of the bit, and would count as a new ownership scheme.  Maybe having these fixed up structs stored on the rule node would be sufficiently similar to how we handle reset structs with conditions that it wouldn't be considered a new ownership scheme!)

Would the style fixup option for solving this bug need you to add another style context bit to track whether we're in a display:ruby-text or display:ruby-text-container subtree?  I am just wondering now whether it would be better to go for the style fixup option, given that we don't actually need to perform any fixup unless we were affected by the minimum font size pref in ComputeFontData, which ought to be the common case.  WDYT?  Or do you worry the memory usage would be unacceptable if a user does happen to run into their minimum font size limits with ruby text?

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) from comment #15)
> Would the style fixup option for solving this bug need you to add another
> style context bit to track whether we're in a display:ruby-text or
> display:ruby-text-container subtree?

Yes, I think we would need a new style context bit to track the rt/rtc subtree. I'm not a fan of doing that, though.

> I am just wondering now whether it
> would be better to go for the style fixup option, given that we don't
> actually need to perform any fixup unless we were affected by the minimum
> font size pref in ComputeFontData, which ought to be the common case.  WDYT?
> Or do you worry the memory usage would be unacceptable if a user does happen
> to run into their minimum font size limits with ruby text?

TBH, I don't really worry about the memory usage, since we've already been creating unique border/padding struct for *every* rbc and rtc (which I hope to fix in bug 1211312).

So it seems it is a doable approach.

Also this approach would make this bug depend on bug 1139283 again (which might be a good news).

Comment 17

[Cameron McCormack (:heycam)]

I think I'm complicating things.  Let's take your internal-property patches, and if we decide we need improved handling of minimum text size in ruby later, e.g. to handle non-<rt> display:ruby-text, we can do it then.

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8668804 [details]
MozReview Request: Bug 1165538 part 2 - Add -moz-min-font-size-ratio internal property.

https://reviewboard.mozilla.org/r/21051/#review19293

::: layout/style/nsRuleNode.cpp:3783
(Diff revision 1)
> +      float percent = minFontSizeRatio->GetPercentValue() * 100;

Can you add a short comment pointing out that while percentages are parsed as floating point numbers, we only store an integer in the range [0, 255], since that's all we need for now.

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8668805 [details]
MozReview Request: Bug 1165538 part 3 - Apply -moz-min-font-size-ratio to rt elements and add test.

https://reviewboard.mozilla.org/r/21053/#review19295

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3804a2ddf72c

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd9e2608f5f8685d423e1949ed17c1a1c2908b3
Bug 1165538 part 1 - Use delegated constructor for nsStyleFont to simplify code and remove the need of nsStyleFont::Init. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a332ac1354b51fd668cdde8f0e84b610396339e
Bug 1165538 part 2 - Add -moz-min-font-size-ratio internal property. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/49ecda81e80df535780df43af423b7747c5c8845
Bug 1165538 part 3 - Apply -moz-min-font-size-ratio to rt elements and add test. r=heycam

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dd9e2608f5f
https://hg.mozilla.org/mozilla-central/rev/1a332ac1354b
https://hg.mozilla.org/mozilla-central/rev/49ecda81e80d