1100912 - Directionality state is propagated in the light tree, not the flattened tree (requires spec)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Wilson Page [:wilsonpage]]

EXPECTED

`:dir()` selector should reflect the closest ancestor's `dir` attribute, even ancestors outside the shadow-host.

ACTUAL

`:dir()` selector only reflects ancestor `dir` attributes inside the shadow-root.

DEMO

http://jsbin.com/talite/1/

Comment 1

[Wilson Page [:wilsonpage]]

*'outside the shadow-root'

Comment 2

[Boris Zbarsky [:bzbarsky]]

> even ancestors outside the shadow-host.

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 on the fact that the rendering behavior Chrome and Firefox have here doesn't match what the spec says.

You're right that we're at least not being internally consistent, in that our rendering behavior seems to treat the shadow DOM as having the directionality of the host but the :dir matching does not... but as I said in that spec bug it's not clear to me why the rendering behavior is what it is, and that it's even correct.

Comment 3

[Cameron McCormack (:heycam)]

Simon, I'm guessing that we don't correctly propagate the directionality content flags down into shadow trees.  Do you know if that should be sufficient here, and whether that is indeed the desired behaviour?

Comment 4

[Simon Montagu :smontagu]

I'd like to investigate this further but the test case here and in https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 fail for me with "TypeError: div.createShadowRoot is not a function". What am I lacking?

From inspecting the testcases I'm also not clear how they determine whether rendering is LTR or RTL.

My own expectation FWIW is that a shadow root should act like a root and not inherit direction from its container, and certainly that it should be bidi-isolated, but that's just an off the cuff reaction and I haven't looked into it in depth.

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Simon Montagu :smontagu from comment #4)
> I'd like to investigate this further but the test case here and in
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 fail for me with
> "TypeError: div.createShadowRoot is not a function". What am I lacking?

Maybe you need to enable web components from about:config ? The pref is "dom.webcomponents.enabled".

Comment 6

[Sam Foster [:sfoster] (he/him)]

From the w3c spec bug https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 '[we] prefer "dir" to be handled the same way as CSS inherits.' While direction is conceptually distinct from style, I read the discussion there as arriving at concensus that it *should* be inherited down into the shadow root in the same way that CSS properties are. Which I think means that :wilsonpage's assertion in Comment #1 stands and his test case is valid. Is that correct, and if so can we get a plan for fixing this bug?

Comment 7

[Boris Zbarsky [:bzbarsky]]

The spec got clarified, yes.  See comment 3 for what needs to be done to implement that.  All that's needed is someone with time to do it.

Comment 8

[Fabien Cazenave [:kaze]]

TL;DR: this bug makes it very difficult to make BiDi-friendly web components for Gaia.

(In reply to Simon Montagu :smontagu from comment #4)
> My own expectation FWIW is that a shadow root should act like a root and not
> inherit direction from its container, and certainly that it should be
> bidi-isolated

In my opinion, this is not suitable from the front-end development point of view. Many native HTML elements have a different appearance in RTL, e.g. <progress> bars are mirrored horizontally, and we need :dir() selectors to implement some front-end details properly.

Front-end developers can cheat and apply `dir` attributes to elements of the shadow DOM when the web component is created, but this can’t work for elements that are dynamically created. Here’s a typical header bar with a “back” button (that should be mirrored in RTL):

    var div = document.createElement('div');
    div.innerHTML = '<gaia-header action="back"><h1>Gaia Header</h1></gaia-header>';
    document.body.insertBefore(div.firstElementChild, anchor);

in this example, our implementation initializes a <gaia-header> instance in line 2, but the direction of this component could be changed in line 3, depending where the component is inserted in the document. And AFAIK there’s no way to work around this with front-end JS.

Comment 9

[Julien Wajsberg [:julienw]]

In the past, we did workarounds like [1]: listen to attribute mutations on <html> so that we can react. Clearly we're depending on a specific behavior in Gaia here.

[1] https://github.com/gaia-components/gaia-header/pull/11/files#diff-c724cebc352c5f85a50270d4af6309d0R245

Comment 10

[Jim Porter (:squib)]

Tricks like comment 9 don't seem to work for web components used inside *other* web components. It would be really nice if we could fix this. I don't mind doing it myself if someone points me in the right direction. I have plenty of experience with C++, and know a bit about Gecko dev from working on Thunderbird for several years, but I haven't done anything in Gecko proper.

Comment 11

[Jim Porter (:squib)]

Boris: do you have any pointers for where I should start looking? I think I could probably handle most of this by myself if I knew where to start.

Comment 12

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

My guess would be that you want to change RecomputeDirectionality in dom/base/DirectionalityUtils.cpp to use GetParentElementCrossingShadowRoot() instead of GetParentElement().  That might not be the only thing that needs to change, though.

There would be some other changes needed to make dir=auto cross shadow root boundaries in other parts of DirectionalityUtils.cpp and maybe elsewhere, probably in both directions (getting parents and getting children).  I'm not sure whether dir=auto is supposed to work that way or not.

But others might be able to comment more definitively.

Comment 13

[Jim Porter (:squib)]

Taking this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

David is right that you need to change RecomputeDirectionality, but it's not clear to me whether it should use GetParentElementCrossingShadowRoot or GetFlattenedTreeParent.  Those have different observable behavior in cases like this:

  <x-foo dir="rtl">
    <span></span>
  </x-foo>

and a shadow tree attached to the x-foo that does:

   <p dir="ltr">
     <children/> <!-- Or whatever the current syntax is -->
   </p>

so that the flattened tree looks like:

   <div dir="rtl">
     <p dir="ltr">
       <span></span>
     </p>
   </div>

Using GetParentElementCrossingShadowRoot() will give the <span> a directionality of "rtl", but using GetFlattenedTreeParent() will give it "ltr".  I suspect you actually want GetFlattenedTreeParent().

You will also need to change SetDirectionalityOnDescendants to walk the flattened tree using FlattenedChildIterator, presumably.  Not sure what should happen for nodes that appear in the DOM but not the flattened tree; I guess you get to check the spec for that and hope whoever wrote it actually defined the behavior.

Making "auto" work will need changes to WalkAncestorsResetAutoDirection (again, to replace GetParentElement()) and The various WalkDescendants* methods in DirectionalityUtils.

And you'll need to make sure that (1) SetDirOnBind happens correctly for all the things we care about and (2) changes to insertion points that change the flattened tree somehow recompute directionalities as needed.  I haven't thought through what's involved in this so far.

Make sure to write lots of tests, especially for dynamic update cases....

Comment 15

[Jim Porter (:squib)]

I'm not likely to work on this anytime soon. Unassigning.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

This is actually more of a DOM thingie.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case updated for Shadow DOM v1.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Actually in chrome the test-case in comment 17 doesn't work at all because they don't support dir, presumably.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

HTML spec says
"Otherwise, the directionality of the element is the same as the element's parent element's directionality."
But element's parent isn't actually defined. Which parent.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I guess that is DOM parent node.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

As far as I see, this is totally unclear from the spec. (or I've missed to look at some other place defining this)
Filed https://github.com/whatwg/html/issues/3699

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(Removing dependency from bug 1405937 since directionality state is a DOM thing.)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Since no one has managed to come up with an algorithm which would implement the behavior people actually want, I'll upload a patch which mimics blink's behavior in most cases (follows flattened-ish tree model).

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_directionality_with_test.diff

This is a nightmare patch to make the nightmare code DirectionalityUtils to deal with ShadowDOM in a way which isn't specified anywhere :)

The patch tries to mimic flattened tree in DOM. So dir state propagates through shadow DOM and from slots back to light dom child nodes.
dir=auto detection on the other hand propagates from assigned nodes and shadow DOM up to the host.

and :dir(rtl)/:dir(ltr) states are propagated too. (Other engines don't support :dir pseudoclass yet.)

I should write more tests and I'm sure there are plenty bugs, like directionality handling always has, but trying to get something landed sooner than later
(and I should be on vacation), and then improve it later.

(DirectionalityUtils in general could use a total rewrite, but not worth right now since it is expected that Shadow DOM's dir handling will change - well - be specified at least in some way.)

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/5907c5d7157f05a49da06c0cef662d1a21bbf5b5
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=5907c5d7157f05a49da06c0cef662d1a21bbf5b5
remote: recorded changegroup in replication log in 0.052s

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8992228 [details] [diff] [review]
shadow_directionality_with_test.diff

Review of attachment 8992228 [details] [diff] [review]:
-----------------------------------------------------------------

I think I understand enough to r+ this.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_directionality_with_test_2.diff

ops, one line change in the existing code caused some crashes
That was done while debugging new code.
SetAncestorDirectionIfAuto shouldn't use SubtreeRoot but keep using parent.

Let's see if this is more tryserver happy. Just that one change.

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/5eda3cc6054270e833017b3addba725c869aec16
remote:   https://hg.mozilla.org/try/rev/763b5cdf02e7da07a3b30b29611f38d660427dbc
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=763b5cdf02e7da07a3b30b29611f38d660427dbc
remote: recorded changegroup in replication log in 0.007s

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a20a23b5e7
make dir-state to propagate through shadow DOM, r=mrbkap

Comment 28

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/18a20a23b5e7