1197765 - Ruby autohiding doesn't work with pseudo ruby base/text box

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Some testcases:
data:text/html,<ruby>a<rt>a</rt></ruby>
data:text/html,<ruby><rb>a<rb>b<rtc>a<rt>b</rtc></ruby>

This is because we get text from the content node of the frame, which, for pseudo boxes, is the parent nodes.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

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

[per IRC discussion w/ xidorn, I'm punting this review request to dbaron, since he reviewed the Ruby autohiding feature in bug 1052123 & hence knows more of the background here, and also since I'm about to leave on vacation.

Using "needinfo?" since dbaron's not accepting review requests right now, so that this is on his queue when he returns from travel. :)]

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

So this change doesn't really match the spec, because the spec reads to me that we shouldn't skip "display: none" items inside the content, nor should the text inside <textarea>. But I think those are edge case and don't really need to care a lot.

I thought about extracting DOM range from frame, and get text content inside the range. This could be more spec-conform, but may have issue with ::before/::after pseudo element, because it seems to me those elements have content node, but the nodes are not in the normal node tree, which may cause issue for traversing.

Comment 4

[Daniel Holbert [:dholbert]]

[apologies for any CC/un-CC bugspam I just caused; I was trying to figure out how to un-CC myself with bugzilla's experimental future UI, and the only obvious method [a "Stop Following" button] didn't seem to be working -- at least, the CC list shown on the bug didn't update immediately.  I pressed the button a few times to experiment, which apparently generated some bugspam, as I later discovered.]

Comment 5

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

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

>+  /**
>+   * Get the text content inside the frame. This methods traverse the
>+   * frame tree and collect the content from text frames. Thus it skips
>+   * any text content which is not included that way, including style,
>+   * script, textarea, and elements with display property set to none.
>+   */
>+  static void GetFrameTextContent(nsIFrame* aFrame, nsAString& aResult);

Please add a comment here explaining how this differs from nsContentUtils::{Get,Append}NodeTextContent.  Differences seem to include at least (1) different handling of pseudos (2) skipping of out-of-flows (3) probably other things.  You don't necessarily need the list to be exhaustive.

Please also add a code comment in nsContentUtils.h pointing to this method.

>diff --git a/layout/reftests/w3c-css/submitted/ruby/ruby-autohide-001-ref.html b/layout/reftests/w3c-css/submitted/ruby/ruby-autohide-001-ref.html
>--- a/layout/reftests/w3c-css/submitted/ruby/ruby-autohide-001-ref.html
>+++ b/layout/reftests/w3c-css/submitted/ruby/ruby-autohide-001-ref.html

Instead of modifying the existing test, please create a new test (hg copy of old test) and make the modifications there, unless there's a good reason that we don't want the existing test.

r=dbaron with that

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4ba4b6e7c95

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4ba4b6e7c95