1225018 - Properly handle leading for text emphasis marks

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

We may also need to handle the alignment of caret and bullet.

Some locations we may need to fix:
https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/layout/forms/nsTextControlFrame.cpp#162-164
https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/layout/forms/nsTextControlFrame.cpp#522-523
https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/layout/generic/nsBRFrame.cpp#128-131
https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/layout/generic/nsBlockFrame.cpp#517-518
https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/layout/generic/nsBlockFrame.cpp#2600-2603
https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/layout/generic/nsLineLayout.cpp#2283-2286

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(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 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?).

Comment 10

[Daniel Holbert [:dholbert]]

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 11

[Daniel Holbert [:dholbert]]

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.)

Comment 12

[Daniel Holbert [: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.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(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 16

[Jonathan Kew [:jfkthame]]

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

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/835de4b7bcb2b9016ef425db25878929c28a9448
Backed out 8 changesets (bug 1225018, bug 1229278, bug 1224013) for reftest failures on CLOSED TREE

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 26

[Jonathan Kew [:jfkthame]]

(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 27

[Jonathan Kew [:jfkthame]]

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?

Comment 28

[Jonathan Kew [:jfkthame]]

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

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

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 35

[Jonathan Kew [:jfkthame]]

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

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

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d3c819671be
https://hg.mozilla.org/mozilla-central/rev/3b472ae8fc26
https://hg.mozilla.org/mozilla-central/rev/384614be367e
https://hg.mozilla.org/mozilla-central/rev/bc1562f01b6a
https://hg.mozilla.org/mozilla-central/rev/56cf53f1d5e2