Closed Bug 1165538 Opened 5 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- affected
firefox44 --- fixed

People

(Reporter: masayuki, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
> 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)
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 :(
No longer depends on: 1139283
Depends on: 1069192
Bug 1165538 part 1 - Use delegated constructor for nsStyleFont to simplify code and remove the need of nsStyleFont::Init.
Attachment #8668803 - Flags: review?(cam)
Bug 1165538 part 2 - Add -moz-min-font-size-ratio internal property.
Attachment #8668804 - Flags: review?(cam)
Bug 1165538 part 3 - Apply -moz-min-font-size-ratio to rt elements and add test.
Attachment #8668805 - Flags: review?(cam)
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?
(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?
Flags: needinfo?(quanxunzhen)
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".
Attachment #8668803 - Flags: review?(cam) → review+
(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.
Flags: needinfo?(quanxunzhen)
(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...
(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?
(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
(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?
(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).
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 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.
Attachment #8668804 - Flags: review?(cam) → review+
Attachment #8668805 - Flags: review?(cam) → review+
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
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
Assignee: nobody → quanxunzhen
You need to log in before you can comment on or make changes to this bug.