[dir=auto] doesn't detect rtl text inside Native Anonymous Content

RESOLVED FIXED in Firefox 61

Status

()

defect
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bgrins, Assigned: timdream)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed, firefox62 wontfix)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

a year ago
This is something I noticed in Bug 1446830. I'll push up a patch to make it easier to reproduce.
Comment hidden (mozreview-request)
Reporter

Comment 2

a year ago
Ehsan, this is a more minimal testcase of what I'm seeing from Bug 1446830. Does this reproduce for you?

STR:
* Apply the patch
* ./mach run --devtools --setpref devtools.inspector.showAllAnonymousContent=true --setpref devtools.inspector.showUserAgentStyles=true 'data:text/html,<input type="file" />'
* Pick a file with RTL text

When I inspect the label in devtools I see:

```
-moz-dir-attr-like-auto:dir(ltr) {
    direction: ltr;
}
```

Something interesting is that if I open `data:text/html,<input dir="rtl" type="file" />` and then inspect the "Browse" button, it does properly detect LTR directionality. Wondering if this has to do with the text being set initially vs being updated dynamically.
Flags: needinfo?(ehsan)

Comment 3

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Something interesting is that if I open `data:text/html,<input dir="rtl"
> type="file" />` and then inspect the "Browse" button, it does properly
> detect LTR directionality. Wondering if this has to do with the text being
> set initially vs being updated dynamically.

That the bug happens only when the text is set dynamically makes perfect sense, and is a *strong* indication that there is a bug somewhere in DirectionalityUtils.cpp code which confuses us when dealing with native anonymous content.

Andrew, any chance you could help find an owner for this to help unblock Brian please?  Thanks.
Flags: needinfo?(ehsan) → needinfo?(overholt)
The only mention within DirectionalityUtils.cpp on NAC is the change made in bug 839886, in DoesNotParticipateInAutoDirection(). My guess is that aElement->IsInAnonymousSubtree() check should move to DoesNotAffectDirectionOfAncestors() and/or NodeAffectsDirAutoAncestor(), since it looks like what we want here is to have <button dir=auto> have it's own direction.
... and bug 1377826 but it might be less related.
Comment hidden (obsolete)
Comment hidden (obsolete)
This is a more involved patch that does what's needed for comment 1 to work. But it basically reverted bug 1377826, so we will need to dig into layout to find an alternative fix to bug 1377826.
Seems Tim is heroically taking a look here.
Flags: needinfo?(overholt)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
The attached patch now works with Brian's demo here, and my second cset that adds dir=auto to the button on <input type=file>. It also limits itself enough to not crash the test case in bug 1377826. I still don't think this is the exhaust fix, because when I try to add dir=auto to the other place I think we need -- the subtitle track list items [1] -- I was met with "These should always be in sync!" assertion error [2].

[1] https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/toolkit/content/widgets/videocontrols.xml#1658
[2] https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/dom/base/nsINode.cpp#313

On the other hand, if this is good enough to unblock bug 1446830, it should be good enough -- I don't think we need an auto-direction implementation that works on every bit of anonymous content, given that we control the markup (or, do we?).

Brian, could you tell me where you intend to set dir=auto in bug 1446830? I didn't find it in the cset you posted.
Comment on attachment 8968467 [details]
Bug 1451576 - Allow dir=auto to work in native anonymous content

Ehsan, could you tell me if I am going in the right direction here? Let me know if you couldn't and someone else could. Thanks.
Attachment #8968467 - Flags: feedback?(ehsan)
Reporter

Comment 14

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> On the other hand, if this is good enough to unblock bug 1446830, it should
> be good enough -- I don't think we need an auto-direction implementation
> that works on every bit of anonymous content, given that we control the
> markup (or, do we?).
> 
> Brian, could you tell me where you intend to set dir=auto in bug 1446830? I
> didn't find it in the cset you posted.

Pushed up the latest version of the patch there. The idea with that version is the markup in the NAC would look something like:

```
<div>
  <button dir="auto" />
  <div>
    <label dir="auto">
      <span>${aValue}</span>                <-- First half (when overflowing)
      <span><span>${aValue}</span></span>   <-- Second half (when overflowing)
      ${aValue}                             <-- Full string (when not overflowing)
    </label>
  </div>
</div>
```

But we could change it as needed to work with whatever fix makes sense here.
Comment hidden (mozreview-request)
Comment on attachment 8968467 [details]
Bug 1451576 - Allow dir=auto to work in native anonymous content

The markup in comment 14 will also trigger assertion failure described in comment 12.

I look it up a bit but I don't understand the code enough to tell why it happens. I couldn't find anything obvious where HasDirAuto could affect the way we call CharacterData::UnbindFromTree().

When it happens, |slowNode| points to the node with dir=auto and |node| points to the outer native anon root <div>. This explains why if the element set with dir=auto is happen to be the direct children of <input> (i.e. they are native anonymous roots), we won't hit this assertion.
Attachment #8968467 - Flags: feedback?(ehsan)
Reporter

Comment 17

a year ago
FWIW - if it's easier to deal with it might be possible to modify the markup/css so that it looks more like:

  <button dir="auto" />
  <div dir="auto">
    <label>
      <span>${aValue}</span>                <-- First half (when overflowing)
      <span><span>${aValue}</span></span>   <-- Second half (when overflowing)
      ${aValue}                             <-- Full string (when not overflowing)
    </label>
  </div>

So both the button and div would be direct children of the host element. The reason I currently have the outer div is so we can use flexbox to make the inner div stretch. There may be another way to accomplish that without introducing the container div.
(In reply to Brian Grinstead [:bgrins] from comment #17)
> FWIW - if it's easier to deal with it might be possible to modify the
> markup/css so that it looks more like:

If that's possible that would be great. I can update the patch a bit to do an extra check.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8968467 - Flags: review?(ehsan)
Attachment #8969232 - Flags: review?(ehsan)
Attachment #8969233 - Flags: review?(ehsan)
I think I've figured it out. Please read my proposed commit messages.

Dealing with the execution order of procedural code on a declarative tree feels like a recurring theme...
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I've manually confirmed the patch posted work with the proposed markup in comment 14. It works because ${aValue} (the file name) happens to be the same even if the dir=auto node is "complex".

```
<div>
  <button dir="auto">                       <-- dir=auto node
    Browse...                               <-- direct children affect the directionality of parent
  </button>
  <div>
    <label dir="auto">                      <-- dir=auto node
      <span>${aValue}</span>                <-- does *not* affact the parent (but does inherit the directionality from parent)
      <span><span>${aValue}</span></span>   <-- does *not* affact the parent (but does inherit the directionality from parent)
      ${aValue}                             <-- direct children affect the directionality of parent
    </label>
  </div>
</div>
```
Just a random note, DirectionalityUtils has proved to be crazy crash-prone. So be exceptionally careful when modifying it.
(I think we should somehow rewrite the whole DirectionalityUtils at some point, but I don't have too good ideas what the new implementation should look like.)
I agree. See comment 22. Hopefully, the proposed patch here, which tailor toward the exactly limited use cases we need in NAC, could prevent me from hitting these crashes.

Comment 35

a year ago
mozreview-review
Comment on attachment 8969232 [details]
Bug 1451576 - Add dir=auto to subtitle menu items in video control

https://reviewboard.mozilla.org/r/237968/#review245486

::: commit-message-2857c:5
(Diff revision 4)
> +Bug 1451576 - Add dir=auto to subtitle menu items in video control
> +
> +The labels of the subtitle track selections come from the web content,
> +from <track label="...">. Given that it will likely be the name of the
> +script in its native script, each of the item should have its own

What's "script" here?  Did you mean language?

::: commit-message-2857c:10
(Diff revision 4)
> +script in its native script, each of the item should have its own
> +directionality, instead of inheriting direction: ltr set on the entire
> +<xul:videocontrol> parent element.
> +
> +I do however wonders if the web content should have the opportunity to
> +affect the directionality of these labels?

Yes, why wouldn't it?  If the user is reading the page in their language, they're expecting it to show up with a sensible direction...

I generally consider any place in our UI where we display a string from a web page without using dir=auto a bug.
Attachment #8969232 - Flags: review?(ehsan) → review+

Comment 36

a year ago
mozreview-review
Comment on attachment 8969233 [details]
Bug 1451576 - Set dir=auto on [Browse...] button of <input type=file>

https://reviewboard.mozilla.org/r/237970/#review245492
Attachment #8969233 - Flags: review?(ehsan) → review+
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

a year ago
mozreview-review
Comment on attachment 8968467 [details]
Bug 1451576 - Allow dir=auto to work in native anonymous content

https://reviewboard.mozilla.org/r/237160/#review245496

I think the limitation in comment 32 is probably OK for the current use cases we have here...  r=me with that in mind.  Thanks for the patch!

::: dom/base/DirectionalityUtils.cpp:285
(Diff revision 6)
>  inline static bool
>  NodeAffectsDirAutoAncestor(nsINode* aTextNode)
>  {
>    Element* parent = aTextNode->GetParentElement();
> +  // The text node inside an anonymous subtree only affects
> +  // the direct dir=auto parent in the same anonymous subtree.

This comment reads as if this is how things should work, but I think you meant to explain the limitation in our implementation.  Do you mind rewording this please to make it read like we currently only support this case?

::: dom/base/DirectionalityUtils.cpp:935
(Diff revision 6)
>  
>    Directionality dir = GetDirectionFromText(aTextNode->GetText());
> -  if (dir != eDir_NotSet && aTextNode->HasTextNodeDirectionalityMap()) {
> +  if (dir != eDir_NotSet) {
>      nsTextNodeDirectionalityMap::ResetTextNodeDirection(aTextNode, aTextNode);
> +  } else {
> +    nsTextNodeDirectionalityMap::EnsureMapIsClearFor(aTextNode);

Can you please add a comment here like this?

// We used to check NodeAffectsDirAutoAncestor() in this function, but
// stopped doing that since calling IsInAnonymousSubtree()
// too late (during nsTextNode::UnbindFromTree) is impossible and this
// function was no-op when there's no directionality map.

something along those lines.
Attachment #8968467 - Flags: review?(ehsan) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 44

a year ago
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8e51a02b3d48
Allow dir=auto to work in native anonymous content r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c576cede0ed1
Add dir=auto to subtitle menu items in video control r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/09f7885a83ce
Set dir=auto on [Browse...] button of <input type=file> r=Ehsan

Comment 45

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e51a02b3d48
https://hg.mozilla.org/mozilla-central/rev/c576cede0ed1
https://hg.mozilla.org/mozilla-central/rev/09f7885a83ce
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
This should've landed with tests... Also, this causes invariant violations like bug 1467964. You can't just change ancestors' direction from Layout, because it affects the style of the element, which can affect the layout itself.

Comment 47

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/194ef402a7b1
Revert three changesets for causing bug 1467964 and since it's not generally sound. r=me
https://hg.mozilla.org/mozilla-central/rev/194ef402a7b1

Does this need backing out from Beta too then?
Status: RESOLVED → REOPENED
Flags: needinfo?(emilio)
Resolution: FIXED → ---
I don't think it's a safety issue as things stand, only a correctness issue, so I talked with Tim yesterday and we're fine not backing this out from beta I think.
Flags: needinfo?(emilio)
This bug should be closed given there is no further action needed.

Not sure I should set this to WONTFIX/INVALID or FIXED. Feel free to correct it.
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.