Properly handle leading for text emphasis marks

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
The spec says
> The effect of emphasis marks on the line height is the same as for ruby text.

However, emphasis marks are much more tricky than ruby. Ruby has its own inline boxes, which limits its usage, and reduces the cases need to handle. Emphasis marks are eventually applied on text, whose line height calculation could affect more places.

It seems we need to handle the following cases in addition to what was for ruby:
1. when the text frame is put directly in a block
2. the size of br
3. text control (intrinsic size, baseline)

We would probably need to make nsLayoutUtils::GetCenteredFontBaseline() take the emphasis marks into account when calculating the baseline position.
Summary: Properly handle leadings for text emphasis marks → Properly handle leading for text emphasis marks
(Assignee)

Comment 2

3 years ago
Created attachment 8693967 [details]
MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron

Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation.
Attachment #8693967 - Flags: review?(dbaron)
(Assignee)

Comment 3

3 years ago
Created attachment 8693968 [details]
MozReview Request: Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

Bug 1225018 part 2 - Ensure leading for emphasis marks of text directly inside block.
Attachment #8693968 - Flags: review?(dholbert)
(Assignee)

Comment 4

3 years ago
Created attachment 8693969 [details]
MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils.

Bug 1225018 part 3 - Handle line height of multiple-line text controls for emphasis marks.
Attachment #8693969 - Flags: review?(dholbert)
(Assignee)

Comment 5

3 years ago
Created attachment 8693970 [details]
MozReview Request: Bug 1225018 part 5 - Add reftests for line height handling of text-emphasis. r=dholbert

Bug 1225018 part 4 - Add reftests for line height handling of text-emphasis.
Attachment #8693970 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Blocks: 1229278
(Assignee)

Updated

3 years ago
Blocks: 1229424
Comment on attachment 8693967 [details]
MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron

https://reviewboard.mozilla.org/r/26627/#review24201

::: layout/style/nsStyleStruct.cpp:3665
(Diff revision 1)
> +    return NS_STYLE_HINT_REFLOW;

I'd prefer new code use the nsChangeHint_* constants rather than NS_STYLE_HINT_* constants.  So in this case I think you want nsChangeHint_RepaintFrame | nsChangeHint_AllReflowHints

r=dbaron with that, although there should also be a separate bug on using UpdateSubtreeOverflow rather than UpdateOverflow, to avoid triggering both performance problems (long lists of hints generated for every element in a subtree) and assertions about generating hints for non-element frames
Attachment #8693967 - Flags: review?(dbaron)
(Assignee)

Comment 7

3 years ago
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #6)
> Comment on attachment 8693967 [details]
> MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis
> changes for line height calculation.
> 
> https://reviewboard.mozilla.org/r/26627/#review24201
> 
> ::: layout/style/nsStyleStruct.cpp:3665
> (Diff revision 1)
> > +    return NS_STYLE_HINT_REFLOW;
> 
> I'd prefer new code use the nsChangeHint_* constants rather than
> NS_STYLE_HINT_* constants.  So in this case I think you want
> nsChangeHint_RepaintFrame | nsChangeHint_AllReflowHints
> 
> r=dbaron with that,

So this is effectively an r+ right?

> although there should also be a separate bug on using
> UpdateSubtreeOverflow rather than UpdateOverflow, to avoid triggering both
> performance problems (long lists of hints generated for every element in a
> subtree) and assertions about generating hints for non-element frames

That is bug 1229278.
Comment on attachment 8693967 [details]
MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron

https://reviewboard.mozilla.org/r/26627/#review24203
Attachment #8693967 - Flags: review+
Yes, I just forgot to check the box, and forgetting to take any action in MozReview causes the flag to be cleared (unlike Bugzilla's UI, where it stays r?).
(Assignee)

Updated

3 years ago
Blocks: 1229609
Attachment #8693970 - Flags: review?(dholbert) → review+
Comment on attachment 8693970 [details]
MozReview Request: Bug 1225018 part 5 - Add reftests for line height handling of text-emphasis. r=dholbert

https://reviewboard.mozilla.org/r/26633/#review24239

::: layout/reftests/w3c-css/submitted/text-decor-3/reftest.list:126
(Diff revision 1)
> +== text-emphasis-line-height-010.html text-emphasis-line-height-001-ref.html

This "010" test seems a bit oddly named, and looks initially like its name is a typo.  After further reading, I suspect it's not a typo & you named it differently to differentiate it from the generated tests.

Perhaps it should really be named "001z", though? (with a comment after it in reftest.list saying "# written by hand, not w/ script", perhaps) If you're trying to name it differently to save this test from getting stomped on in future runs of the script, the "z" letter should make it pretty safe, I'd expect.

(Alternately -- if you want to keep it named "010", maybe add a comment inside the test (say, near its <link rel="match"> tag) saying that this test is named 010 instead of 001\* to more clearly separate it from the script-generated tests, though it does share a reference case with them.)

r=me on the tests, with that.
Comment on attachment 8693969 [details]
MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils.

https://reviewboard.mozilla.org/r/26631/#review24255

::: layout/forms/nsTextControlFrame.cpp:538
(Diff revision 1)
> +  if (!IsSingleLineTextControl()) {

Could you cache the result of the earlier |IsSingleLineTextControl()| call in a const local variable?

It looks like it involves a QI and a virtual function-call, so probably better to avoid needlessly calling it twice in a row.

::: layout/forms/nsTextControlFrame.cpp:549
(Diff revision 1)
> +      ascent = std::max(ascent, fontHeight + emphasisHeight);

Two questions (which may betray my lack of knowledge about nsTextControlFrame):

(1) Why do we adjust |ascent| instead of |lineHeight| here?  Up higher, in CalcIntrinsicSize(), we adjust |lineHeight|. But here, we adjust ascent, which we calculate as a several-layers-removed derivative of lineHeight.  So -- is this inconsistency (RE what we apply the emphasis std::max()-growth to) problematic?

(2) The comment here ("Ensure text emphasis marks are displayed within the control area") seems to say we're trying to avoid any possibility of the emphasis-mark painting outside (or trying to do so). But all we're really doing here is adjusting the ascent of the nsTextControlFrame -- i.e. adjusting how the nsTextControlFrame aligns with stuff around it, I think.  So I don't see how the comment about emphasis-painting actually ties into the ascent-calculation.  (Adjusting the ascent would make more sense to me if this was about positioning the \*text\* inside the textfield -- but I think |ascent| here is for the textfield control itself, right? Not its text.)

::: layout/forms/nsTextControlFrame.cpp:554
(Diff revision 1)
> +  aDesiredSize.SetBlockStartAscent(ascent);

The comment here, "now adjust for our borders and padding", needs moving or removing I think.  (In the pre-patch version of this code, we add border-padding at this spot. Now we do that earlier.)
Attachment #8693969 - Flags: review?(dholbert)
Comment on attachment 8693968 [details]
MozReview Request: Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

Would you mind if I punt part 2 to jfkthame (or someone else who's touched that section of code)?

I'm not really familiar with the contextual code (in nsLineLayout::VerticalAlignFrames), and I don't immediately trust myself to know what's to look out for there.
(Assignee)

Comment 13

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #12)
> Comment on attachment 8693968 [details]
> MozReview Request: Bug 1225018 part 2 - Ensure leading for emphasis marks of
> text directly inside block.
> 
> Would you mind if I punt part 2 to jfkthame (or someone else who's touched
> that section of code)?
> 
> I'm not really familiar with the contextual code (in
> nsLineLayout::VerticalAlignFrames), and I don't immediately trust myself to
> know what's to look out for there.

Yeah, that's fine.
(Assignee)

Comment 14

3 years ago
Comment on attachment 8693968 [details]
MozReview Request: Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26629/diff/1-2/
Attachment #8693968 - Flags: review?(dholbert) → review?(jfkthame)
(Assignee)

Comment 15

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #11)
> Comment on attachment 8693969 [details]
> MozReview Request: Bug 1225018 part 3 - Handle line height of multiple-line
> text controls for emphasis marks.
> 
> https://reviewboard.mozilla.org/r/26631/#review24255
> 
> ::: layout/forms/nsTextControlFrame.cpp:538
> (Diff revision 1)
> (2) The comment here ("Ensure text emphasis marks are displayed within the
> control area") seems to say we're trying to avoid any possibility of the
> emphasis-mark painting outside (or trying to do so). But all we're really
> doing here is adjusting the ascent of the nsTextControlFrame -- i.e.
> adjusting how the nsTextControlFrame aligns with stuff around it, I think. 
> So I don't see how the comment about emphasis-painting actually ties into
> the ascent-calculation.  (Adjusting the ascent would make more sense to me
> if this was about positioning the \*text\* inside the textfield -- but I
> think |ascent| here is for the textfield control itself, right? Not its
> text.)

Ah... yeah, you're right. Then I guess we probably don't need the Reflow part of this patch. I believe vertical aligning a multiline text control by baseline is a rare case, and applying text-emphasis to text control itself is already a rare case, so I don't think we need to care this a lot.

In addition, this seems to be a more general issue that we calculate ascent for text control independently instead of using the ascent from its inner block frame. I guess we eventually want to use the ascent of the inner block, so that we don't need anything special here.
Comment on attachment 8693968 [details]
MozReview Request: Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

https://reviewboard.mozilla.org/r/26629/#review24433
Attachment #8693968 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 17

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e543f76de8176d338bf96937edc88779ba061f78
Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2bfc7f95ba2c944f65144ef187a485d920946b
Bug 1225018 part 2 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/15e427d8d6eafcb8832600e40d4d58c88adf385b
Bug 1225018 part 3 - Add reftests for line height handling of text-emphasis. r=dholbert
(Assignee)

Updated

3 years ago
Assignee: nobody → quanxunzhen
(Assignee)

Comment 19

3 years ago
The reftests fail on B2G and Mulet because the font for Japanese text does not contain glyphs for the dots, and thus another font kicks in there.

But I'm confused. IIUC, the block size of an inline frame is also derived from the font setting directly, and keeps consistent no matter whether fallbak happens, is that correct? 

Ruby uses the block size of the ruby text frame as its required leading, and emphasis marks use half of the max height of the font metrics as its required leading. So this failure probably means 50% max height of a font may not equal to max height of a 50% font?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 20

3 years ago
OK I probably know the reason. So basically, the block size of the ruby text frame is 50% of the max height. However, in the font setting of Mulet and B2G, with default font size, 50% max height is not integral in pixel, and at some point, the number is rounded for ruby, so the difference presents. I haven't had good idea how to work around this, though.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 21

3 years ago
Comment on attachment 8693967 [details]
MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26627/diff/1-2/
Attachment #8693967 - Attachment description: MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. → MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron
(Assignee)

Comment 22

3 years ago
Comment on attachment 8693969 [details]
MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26631/diff/1-2/
Attachment #8693969 - Attachment description: MozReview Request: Bug 1225018 part 3 - Handle line height of multiple-line text controls for emphasis marks. → MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils.
Attachment #8693969 - Flags: review?(jfkthame)
(Assignee)

Comment 23

3 years ago
Created attachment 8695833 [details]
MozReview Request: Bug 1225018 part 3 - Use font metrics of emphasis marks to compute required leading. r=jfkthame

Bug 1225018 part 3 - Use font metrics of emphasis marks to compute required leading.
Attachment #8695833 - Flags: review?(jfkthame)
(Assignee)

Comment 24

3 years ago
Comment on attachment 8693968 [details]
MozReview Request: Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26629/diff/1-2/
Attachment #8693968 - Attachment description: MozReview Request: Bug 1225018 part 2 - Ensure leading for emphasis marks of text directly inside block. → MozReview Request: Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame
(Assignee)

Comment 25

3 years ago
Comment on attachment 8693970 [details]
MozReview Request: Bug 1225018 part 5 - Add reftests for line height handling of text-emphasis. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26633/diff/1-2/
Attachment #8693970 - Attachment description: MozReview Request: Bug 1225018 part 4 - Add reftests for line height handling of text-emphasis. → MozReview Request: Bug 1225018 part 5 - Add reftests for line height handling of text-emphasis. r=dholbert
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #19)
> So this failure probably means 50% max height of a font
> may not equal to max height of a 50% font?

That wouldn't surprise me. There's some pixel-rounding that get involved in computing the font metrics, so the height/line-spacing may not scale exactly linearly.
Comment on attachment 8693969 [details]
MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils.

https://reviewboard.mozilla.org/r/26631/#review24627

Looking at this, I'm thinking that we really shouldn't be duplicating the whole of GetFontMetricsForStyleContext; can't we let GetFontMetricsOfEmphasisMarks simply be an (inline) wrapper that multiplies the font-inflation parameter by 0.5?
Attachment #8693969 - Flags: review?(jfkthame)
Attachment #8695833 - Flags: review?(jfkthame) → review+
Comment on attachment 8695833 [details]
MozReview Request: Bug 1225018 part 3 - Use font metrics of emphasis marks to compute required leading. r=jfkthame

https://reviewboard.mozilla.org/r/27201/#review24629
(Assignee)

Comment 29

3 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #27)
> Comment on attachment 8693969 [details]
> MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks
> to nsLayoutUtils.
> 
> https://reviewboard.mozilla.org/r/26631/#review24627
> 
> Looking at this, I'm thinking that we really shouldn't be duplicating the
> whole of GetFontMetricsForStyleContext; can't we let
> GetFontMetricsOfEmphasisMarks simply be an (inline) wrapper that multiplies
> the font-inflation parameter by 0.5?

We could not because the orientation computation was different in my initial patch. But since you argued that we should respect sideways in emphasis marks, that logic became same as well. So yes, we should make it a simple wrapper.
(Assignee)

Comment 30

3 years ago
Comment on attachment 8693967 [details]
MozReview Request: Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26627/diff/2-3/
(Assignee)

Comment 31

3 years ago
Comment on attachment 8693969 [details]
MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26631/diff/2-3/
Attachment #8693969 - Flags: review?(jfkthame)
(Assignee)

Comment 32

3 years ago
Comment on attachment 8695833 [details]
MozReview Request: Bug 1225018 part 3 - Use font metrics of emphasis marks to compute required leading. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27201/diff/1-2/
Attachment #8695833 - Attachment description: MozReview Request: Bug 1225018 part 3 - Use font metrics of emphasis marks to compute required leading. → MozReview Request: Bug 1225018 part 3 - Use font metrics of emphasis marks to compute required leading. r=jfkthame
(Assignee)

Comment 33

3 years ago
Comment on attachment 8693968 [details]
MozReview Request: Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26629/diff/2-3/
(Assignee)

Comment 34

3 years ago
Comment on attachment 8693970 [details]
MozReview Request: Bug 1225018 part 5 - Add reftests for line height handling of text-emphasis. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26633/diff/2-3/
Comment on attachment 8693969 [details]
MozReview Request: Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils.

https://reviewboard.mozilla.org/r/26631/#review24707
Attachment #8693969 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 36

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3c819671be0cea51b05f1dd48dd406c141e125
Bug 1225018 part 1 - Trigger reflow on some text emphasis changes for line height calculation. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b472ae8fc261bdebf2d4a62b7b462d1d3ac12f7
Bug 1225018 part 2 - Move GetFontMetricsOfEmphasisMarks to nsLayoutUtils. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/384614be367e5aa1bb87e8c6f667f7fee3305628
Bug 1225018 part 3 - Use font metrics of emphasis marks to compute required leading. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1562f01b6ac3bfe189865e380bcb1becfb051e
Bug 1225018 part 4 - Ensure leading for emphasis marks of text directly inside block. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/56cf53f1d5e26bf486904b1aab4022f31538e80d
Bug 1225018 part 5 - Add reftests for line height handling of text-emphasis. r=dholbert
You need to log in before you can comment on or make changes to this bug.