1107701 - Ignore autohidden annotation when calculating line-height

Status: NEW

(Core :: Layout: Ruby, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

In current code of bug 1052123 and bug 1055665, autohidden annotations would still affect the line-height calculation, which should be avoided.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8573542 [details] [diff] [review]
patch

I should probably defer this review to someone who's more familiar with our inline-layout code (like smontagu or jfkthame or roc), since I don't know nsLineLayout.cpp particularly well.

Having said that, I do have some questions here.

(1) Are you really sure we should ignore autohidden boxes' line-heights?  I don't directly see that in the spec, and it actually seems like this might violate the spec. In particular, the spec says: "Hiding a ruby annotation does not affect [...] the block-axis positioning of boxes in other levels".  It seems to me like this bug's change goes against that. e.g. Suppose we have a ruby annotation that has a huge line-height, and this annotation changes from visible to autohidden, it sounds like your patch here would then remove its "line-height" influence, which (I think) *would* affect the block-axis positioning of boxes in other levels, which violates this line of spec-text.  I could be misunderstanding something, though.

(2) I don't understand why the patch's test-changes are necessary.  Does the test fail without the changes? (& is that a bug?)  Or, are you changing the test to make it test this bug? (If so, it might be better to add a new, modified test with "hg cp" instead, and leave the simpler existing test unmodified)

(3) On the ua.css changes...
>+++ b/layout/style/ua.css
>   *|*::-moz-ruby-text-container {
>     display: ruby-text-container;
>+    /* To avoid affecting line height calculation */
>+    line-height: 0;

...this needs a clearer comment, and I'm wondering:
 - Why do we need this? (on top of the nsLineLayout changes)
 - Does this risk causing text to stomp on top of other text? (That's what "line-height:0" generally does)
 - Why are we adding this to the ruby-text-container (and unconditionally), instead of the ruby-text, which IIUC is the thing that'd be maybe-autohidden?
 - Is it bad that authors can just override this?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 8573542 [details] [diff] [review]
> patch
> 
> I should probably defer this review to someone who's more familiar with our
> inline-layout code (like smontagu or jfkthame or roc), since I don't know
> nsLineLayout.cpp particularly well.
> 
> Having said that, I do have some questions here.
> 
> (1) Are you really sure we should ignore autohidden boxes' line-heights?  I
> don't directly see that in the spec, and it actually seems like this might
> violate the spec. In particular, the spec says: "Hiding a ruby annotation
> does not affect [...] the block-axis positioning of boxes in other levels". 
> It seems to me like this bug's change goes against that. e.g. Suppose we
> have a ruby annotation that has a huge line-height, and this annotation
> changes from visible to autohidden, it sounds like your patch here would
> then remove its "line-height" influence, which (I think) *would* affect the
> block-axis positioning of boxes in other levels, which violates this line of
> spec-text.  I could be misunderstanding something, though.

But the spec says:

the hidden annotation ... has no impact on layout other than to separate adjacent sequences of ruby annotation boxes within its level, as if they belonged to separate segments and the hidden annotation’s base were not a ruby base but an intervening inline. 

It means the line-height of that should be complete ignored.

I think block-axis positioning sentence intends to notice that the annotation in other levels should not fall down to the level to fill the space of the hidden annotation.

> (2) I don't understand why the patch's test-changes are necessary.  Does the
> test fail without the changes? (& is that a bug?)  Or, are you changing the
> test to make it test this bug? (If so, it might be better to add a new,
> modified test with "hg cp" instead, and leave the simpler existing test
> unmodified)

The change is to make it test this bug. The test was falsely fixed by another patch which changes the height calculation method of ruby text container.

> (3) On the ua.css changes...
> >+++ b/layout/style/ua.css
> >   *|*::-moz-ruby-text-container {
> >     display: ruby-text-container;
> >+    /* To avoid affecting line height calculation */
> >+    line-height: 0;
> 
> ...this needs a clearer comment, and I'm wondering:
>  - Why do we need this? (on top of the nsLineLayout changes)

Because the anonymous container inherits the line-height and font-size from its parent, which is usually larger than its children. It doesn't take effect normally, because we have change the line layout to not consider the line-height of that container. But it could still cause problem when we have "vertical-align: top" on its children. In detail, the problem is, if the rtc is anonymous, and a child has the vertical-align, the child will be placed higher than others, however if the rtc is not anonymous, the child will just placed at the same block-axis position with others.

I guess this change is not necessary, and the spec doesn't mention it at all. But I do think this could provide better result.

>  - Does this risk causing text to stomp on top of other text? (That's what
> "line-height:0" generally does)

No because ruby text container has height from their children, and their positions are computed according to the height, not line-height.

>  - Why are we adding this to the ruby-text-container (and unconditionally),
> instead of the ruby-text, which IIUC is the thing that'd be maybe-autohidden?

Well, actually this change is unrelated to auto-hiding. It can be separated from this patch. It was just a problem I found when I played with the test.

>  - Is it bad that authors can just override this?

It is on anonymous box, which cannot be overridden by authors.

TBH, dbaron doesn't like applying styles like this to anonymous boxes.

Comment 4

[Daniel Holbert [:dholbert]]

I think those two spec-sentences are in conflict & need resolving somehow. The sentence I highlighted seems to say that annotations have the same block-axis impact, regardless of whether they're hidden. But the sentence you highlighted seems to say that hidden annotations have no block-axis impact on layout.

Unless I'm misunderstanding, we probably need to clarify that in the spec.