1052123 - Autohide ruby annotations which are identical to their ruby bases

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[sgbowen]

When ruby annotation content is identical to the content of its matching ruby base, the spec requires that the annotation content be hidden (see the URL).

Comment 1

[sgbowen]

Attachment: ruby-autohiding.patch v1

There was a patch for this attached to bug 1030993. I'm attaching that version here for history purposes, but it needs to be rebased or even completely rewritten at this point.

Comment 2

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

(Per Xidorn, no other browsers support autohiding, so moving off the ruby list that blocks enabling the feature onto the other list.)

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 5

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

Are you relying on making the overflow area empty causing us to not paint anything?  That doesn't seem reliable -- would it be better to also make its BuildDisplayList method return early?

How closely does what nsContentUtils::GetNodeTextContent do match the definition of equality used in the spec?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

The spec says:

The content comparison for this auto-hiding behavior takes place prior to white space collapsing (white-space) and text transformation (text-transform) and ignores elements (considers only the textContent of the boxes).

Hence nsContentUtils::GetNodeTextContent exactly matches the spec, as we use this function for element.textContent.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 8

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

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

nsRubyBaseContainerFrame.cpp:

>+      static_cast<nsRubyTextFrame*>(textFrame)->
>+        SetAutoHide(annotationText.Equals(baseText));

Please comment that the spec explicitly says that:
  The content comparison for this auto-hiding behavior takes place prior
  to white space collapsing (white-space) and text transformation
  (text-transform) and ignores elements (considers only the textContent
  of the boxes).
which means that using the content tree text comparison is correct.

nsRubyTextFrame.h:

>+  virtual void BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>+                                const nsRect&           aDirtyRect,
>+                                const nsDisplayListSet& aLists);

Please use MOZ_OVERRIDE.

>+  bool mAutoHide;

Instead of using an additional boolean member, please use a frame
state bit.  (I realize I probably should have had you do this for a
few other things as well.)  See nsFrameStateBits.h, and make sure
to add the bits after the inline bits.

(In this case it's not all that important given that there aren't going
to be a really large number of ruby text frames, but I think it's good
practice.)

nsRubyTextFrame.cpp:

>+  // Even if we want to hide this frame, we have to reflow it first.
>+  // If we leave it dirty, changes to its content will never be
>+  // propagated to the ancestors, then it won't be displayed even if
>+  // the content is no longer the same, until next reflow triggered by
>+  // some other change.

I think you should also comment that in general we always reflow
all the frames we create; there might be other problems that would
result.

>+  if (mAutoHide) {
>+    aDesiredSize.ClearSize();
>+  }

You should also (after the ClearSize()) call
aDesiredSize.SetOverflowAreasToDesiredBounds().

You should also comment that the intent (per spec) is to not affect
layout at all.

This might mean you also need to consider mAutoHide when you
do the line height calculations (so probably worth leaving a note
in that bug).

r=dbaron with those changes, although perhaps I should have a
look at the changes to use a frame state bit

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=dbaron

I guess you should have a look at the state bit. I'm not quite confident about it.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

It seems hard to remove all impacts of the frames once we reflow them. Would it work if we clear their dirty flag recursively without reflowing them?

Comment 11

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

Comment on attachment 8532074 [details] [diff] [review]
patch, r=dbaron

Please add a comment in nsFrameStateBits.h saying that nsRubyTextFrame
inherits from nsInlineFrame (actually, probably 2 comments, one at the end of the nsInlineFrame section, and another at the start of the nsRubyTextFrame section).

>+      // and text transformation (text-transform) and, ignore elements

" and, ignore" -> ", and ignores" (move comma, add s to ignore)


r=dbaron with that

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=430784ec00a5

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50d264e4ffa4

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for linux and android reftest failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/cdefc124ccaa

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4701762&repo=mozilla-inbound

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6a7c8f9552

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d6a7c8f9552

Comment 17

[Patrick Dark]

Attachment: Test Case

This bug doesn't seem to be fixed. (I've attached a test case.)

You have to explicitly declare |display: ruby-base| or use the disallowed |rb| element to get auto-hiding behavior and then it breaks again if you insert characters using the |::before| and |::after| pseudo-elements even though the CSS1 Ruby spec only requires a |textContent| match to trigger it.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

It is HTML spec's fault for not having <rb> element. CSS Ruby encourages using <rb> with <rt>. You should read https://www.w3.org/International/articles/ruby/markup.en.html