[WIP] Implement :dir()

[WIP] Implement :dir()

Design notes: https://docs.google.com/document/d/15lOJZqOKT5X_9HDs1lqN1nvcttsCyd9OrSv50yW6dqY/edit

[Timothy Loh, 2016-04-20 02:19:14]

timloh@chromium.org changed reviewers:
+ esprehn@chromium.org, kojih@chromium.org, rune@opera.com

[Timothy Loh, 2016-04-20 02:19:14]

This was what I was thinking of when we were talking about caching
directionality. I'm not sure it works too well though, so I'm sending this out
first before cleaning up names / writing tests / fixing bugs / etc..

My understanding of everything (correct me if I'm wrong):

https://html.spec.whatwg.org/multipage/dom.html#the-directionality
dir takes on three states. Fixed values (ltr/rtl), auto (depends on subtree **),
and not present or invalid (inherits from parent). A <bdi> element with no dir
attribute behaves as dir=auto.

dirAttributeChanged() is called either when parsing or when we change the dir
attribute. We can't compute the correct dir value if we got here when parsing
because the element won't have it's parent/children set.

childrenChanged() needs to either invalidate or updated the cached
directionality for <bdi>/dir=auto. It also needs to call pseudoStateChanged for
invalidation of :dir() and setNeedsStyleRecalc for direction property changes.

setHasDirAutoAttribute()/selfOrAncestorHasDirAutoAttribute()/etc. is an
optimisation so when dir=auto isn't used we don't slow down childrenChanged()

For the first style resolve, we can either compute directionality lazily (as in
this patch) or hook somewhere I haven't though of and have computed it already
-- computing it in dirAttributeChanged() doesn't work as mentioned above, but
also none of these are called for <bdi> elements without a dir attribute.

If we want to cache directionality, then we can either do this for only
<bdi>/dir=auto or for all elements. If we only do this for <bdi>/dir=auto (i.e.
this CL), then we need to walk the ancestor chain to get the directionality if
dir isn't set (I didn't implement this yet). If we want to do this for all
elements, then we need to find a place to cache it. We can't use ElementData
because is shared between elements with the same local state and we could have
two otherwise identical elements inheriting different directionalities.

So I could make this CL work by making Element::directionality() walk it's
ancestor chain if dir isn't set, but I'm not sure if this is the best option.
Does anyone have better ideas?

** Looks like dir=auto is supposed to inherit from the parent if no child gives
a directionality, but it doesn't look like either us or FF implement this? I
haven't thought about what implications this has yet since I only just noticed
this. https://jsfiddle.net/gx3npmsq/

[rune, 2016-04-20 08:21:29]

On 2016/04/20 02:19:14, Timothy Loh wrote:

> ** Looks like dir=auto is supposed to inherit from the parent if no child
gives
> a directionality, but it doesn't look like either us or FF implement this? I
> haven't thought about what implications this has yet since I only just noticed
> this. https://jsfiddle.net/gx3npmsq/

Yeah, so according to the html spec, every element has a directionality. That
means you'll either have to store it on every element, or walk up the tree to
the closest ancestor where it's stored if you haven't stored it yourself. This
is interesting in combination with inheritance of the direction property. If you
look at the UA stylesheet for HTML, the direction property is set based on the
directionality if you have a dir attribute (or is bdi). That means inheritance
will take care of setting the direction property on elements not having a dir
attribute.

IIUC, the css direction property for the div elements below (outer to inner)
should be: rtl, ltr, ltr, rtl, rtl.

<style>
  #x:dir(rtl) { direction: rtl }
</style>
<div dir="rtl">
  <div style="direction:ltr">
    <div>
      <div id="x">
        <div dir="auto"></div>
      </div>
    </div>
  </div>
</div>

The consequence of supporting :dir() is that you possibly need to know the
directionality of every element in the document for the cases like the
#x:dir(rtl) selector above. With only the UA support for :dir() which we
hardcoded, we only needed to know the directionality of the element when there
was a dir attribute present.

[Timothy Loh, 2016-04-20 13:34:22]

On 2016/04/20 08:21:29, rune wrote:
> Yeah, so according to the html spec, every element has a directionality. That
> means you'll either have to store it on every element, or walk up the tree to
> the closest ancestor where it's stored if you haven't stored it yourself. This
> is interesting in combination with inheritance of the direction property. If
you
> look at the UA stylesheet for HTML, the direction property is set based on the
> directionality if you have a dir attribute (or is bdi). That means inheritance
> will take care of setting the direction property on elements not having a dir
> attribute.

AFAICT the CSS parts are fairly straightforwards, since directionality does not
depend on style. The thing I'm unsure about is how to handle directionality for
the cases where it is inherited (i.e. unset, invalid, or auto in certain cases).
I figure caching it on ElementData means we lose any benefits of the
ElementDataCache if you use :dir(). Caching it persistently between style
resolves means that we also have to hook where an element gets reparented.
Perhaps it could be cached only for the duration of a style resolve?

[Timothy Loh, 2016-04-20 16:19:10]

Ok, I just realised I could cache it on Node. The comment there says there are 3
bits left (actually I count 4??), and I'd need at least two bits (not cached /
ltr / rtl). I'm wary though because this might get in the way of other work
which may need bits here (e.g. separating layout tree updating from style
resolve may need some bits).

[esprehn, 2016-04-20 16:28:24]

I think you want to just walk the ancestors, that's what :lang does. We
don't need to cache this.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-04-20 16:30:42]

I think you want to just walk the ancestors, that's what :lang does. We
don't need to cache this.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Timothy Loh, 2016-05-12 07:59:17]

timloh@chromium.org changed reviewers:
+ kojii@chromium.org
- kojih@chromium.org

[Timothy Loh, 2016-05-12 09:28:02]

I haven't been working on this, but I just rebased it and remembered a problem
in the latest patch I had.

The selfOrAncestorHasDirAutoAttribute flag is stored on ComputedStyle (computed
during style resolve) and is checked in childrenChanged() to determine whether
we should invalidate ancestors. We need to make sure an element with a cached
directionality value is invalidated appropriately. The current patch has a
problem where if the directionality is cached before we have a ComputedStyle,
changing a descendant won't invalidate its ancestors (e.g. patch set 3,
dir-pseudo-auto.html:58 fails).

It doesn't sound like a good idea to me to make childrenChanged() walk its
ancestors if we don't have a ComputedStyle yet since this will slow down
existing code. I think the options are either not caching directionality when we
don't have a ComputedStyle or moving the flag to (e.g.) Element or ElementData.
I don't know think either of these are particularly nice because they both
require more work in making sure things are up to date, and I suspect there may
be some cases with the first option will be wrong (or at least it is more bug
prone).

Does anyone have any other ideas or preferences on how to handle this case?

https://codereview.chromium.org/1898403002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt
(right):

https://codereview.chromium.org/1898403002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt:2:
FAIL ':dir(rtl)' matches all elements whose directionality is 'rtl'.
assert_array_equals: lengths differ, expected 7 got 5
These tests here have several problems and don't indicate issues with the patch.

[rune, 2016-05-12 12:37:34]

On 2016/05/12 09:28:02, Timothy Loh wrote:
> I haven't been working on this, but I just rebased it and remembered a problem
> in the latest patch I had.
> 
> The selfOrAncestorHasDirAutoAttribute flag is stored on ComputedStyle
(computed
> during style resolve) and is checked in childrenChanged() to determine whether
> we should invalidate ancestors. We need to make sure an element with a cached
> directionality value is invalidated appropriately. The current patch has a
> problem where if the directionality is cached before we have a ComputedStyle,
> changing a descendant won't invalidate its ancestors (e.g. patch set 3,
> dir-pseudo-auto.html:58 fails).
> 
> It doesn't sound like a good idea to me to make childrenChanged() walk its
> ancestors if we don't have a ComputedStyle yet since this will slow down
> existing code. I think the options are either not caching directionality when
we
> don't have a ComputedStyle or moving the flag to (e.g.) Element or
ElementData.
> I don't know think either of these are particularly nice because they both
> require more work in making sure things are up to date, and I suspect there
may
> be some cases with the first option will be wrong (or at least it is more bug
> prone).

You cannot both avoid traversal and not have a flag on element since a
display:none descendant may affect the directionality of an element being
displayed/having a ComputedStyle.

This works in Gecko, not in Blink:

<!DOCTYPE html>
<div id="t" dir="auto">
    <span style="display:none">
        <span>XXX</span>
    </span>
</div>
<script>
    console.log(getComputedStyle(t).direction);
    document.querySelectorAll("span")[1].innerHTML = "&#x0632;";
    console.log(getComputedStyle(t).direction);
</script>

With the :dir() selector, you also have the possibility of the directionality of
a display:none element affecting the computed style of a dislayed sibling
through selectors like ":dir(rtl) + span".

How common is dir="auto", btw? Could we skip all the mutation work if there are
no dir="auto" attributes in the document?

[kojii, 2016-05-16 06:25:54]

As esprehn@ commented, couldn't we do just like lang() without creating cache?

Or even better, since we compute CSS direction property from the dir attribute
before matchAllRules(), I had an impression that we could do this during the
style recalc, but I know little about style calc and invalidation, was that
impossible?

On 2016/05/12 at 12:37:34, rune wrote:
> 
> How common is dir="auto", btw? Could we skip all the mutation work if there
are no dir="auto" attributes in the document?

Should not be common, Trident/Edge doesn't implement yet.

httparchive (as of 2014/8/15, unfortunately this is the latest body we have in
bigquery) shows:
  SELECT COUNT(url) FROM [httparchive:runs.2014_08_15_requests_body]
  WHERE REGEXP_MATCH(body, r'dir=["\']?auto') 
=> 542
Number of rows=5,861,260 so this is 0.009%.

[rune, 2016-05-19 09:48:31]

On 2016/05/16 06:25:54, kojii wrote:
> As esprehn@ commented, couldn't we do just like lang() without creating cache?

Yes, if this is rare enough, it seems fine for now.

> Or even better, since we compute CSS direction property from the dir attribute
> before matchAllRules(), I had an impression that we could do this during the
> style recalc, but I know little about style calc and invalidation, was that
> impossible?

Due to the UA stylesheet for HTML, dir="auto" will always need to be resolved
during style resolution. That's probably why it's cached now. If elements with
dir="auto" are few, not caching it across style resolutions seems fine. What
could be really bad, is if we have :dir() selectors in non-rightmost compounds
and we don't cache resolved dir="auto" at all. Then with:

<style>div:dir(rtl) span {}</style>
<div dir="auto"><span></span>...<span></span></div>

you'd resolve dir="auto" for the div as many times as you have span descendants.
A per-style-resolve cache for dir="auto" elements could a good idea. You'd still
avoid the mutation accounting.

[rune, 2016-05-19 09:58:13]

On 2016/05/19 09:48:31, rune wrote:
> On 2016/05/16 06:25:54, kojii wrote:
> > As esprehn@ commented, couldn't we do just like lang() without creating
cache?
> 
> Yes, if this is rare enough, it seems fine for now.
> 
> > Or even better, since we compute CSS direction property from the dir
attribute
> > before matchAllRules(), I had an impression that we could do this during the
> > style recalc, but I know little about style calc and invalidation, was that
> > impossible?
> 
> Due to the UA stylesheet for HTML, dir="auto" will always need to be resolved
> during style resolution. That's probably why it's cached now. If elements with
> dir="auto" are few, not caching it across style resolutions seems fine. What
> could be really bad, is if we have :dir() selectors in non-rightmost compounds
> and we don't cache resolved dir="auto" at all. Then with:
> 
> <style>div:dir(rtl) span {}</style>
> <div dir="auto"><span></span>...<span></span></div>
> 
> you'd resolve dir="auto" for the div as many times as you have span
descendants.
> A per-style-resolve cache for dir="auto" elements could a good idea. You'd
still
> avoid the mutation accounting.

Uh. Never mind. There is still the problem of style invalidations when you have
mutations.

[kojii, 2016-05-19 10:26:47]

Again from who know little about invalidations, please disregard if I'm missing
the point.

On mutation, can't we force style recalc once, then we know whether the element
is in dir=auto or not. If it is, traverse ancestors up to dir=auto, compute dir
and set CSS direction. If it changes, we compute style twice, but not more than
that, no?

[kojii, 2016-05-19 10:39:12]

On 2016/05/19 at 09:48:31, rune wrote:
> <style>div:dir(rtl) span {}</style>
> <div dir="auto"><span></span>...<span></span></div>
> 
> you'd resolve dir="auto" for the div as many times as you have span
descendants. A per-style-resolve cache for dir="auto" elements could a good
idea.

At this moment, my preference is, use CSS direction property to resolve :dir(),
assuming it's possible because of my understanding that matchAllRules() comes
after CSS direction property was resolved.

It violates the spec saying "ignore CSS direction property", but the use of CSS
direction property is strongly discouraged. The case where "auto is used" and
"author sets CSS direction property in their stylesheet" and "the CSS rule
points elements within the subtree of dir=auto" should be rare enough to ignore.

[esprehn, 2016-05-19 21:29:17]

On 2016/05/19 at 10:39:12, kojii wrote:
> On 2016/05/19 at 09:48:31, rune wrote:
> > <style>div:dir(rtl) span {}</style>
> > <div dir="auto"><span></span>...<span></span></div>
> > 
> > you'd resolve dir="auto" for the div as many times as you have span
descendants. A per-style-resolve cache for dir="auto" elements could a good
idea.
> 
> At this moment, my preference is, use CSS direction property to resolve
:dir(), assuming it's possible because of my understanding that matchAllRules()
comes after CSS direction property was resolved.
> 

That's backwards, matchAllRules() is *before* any style is resolved. Also we
need to be able to match rules when there's no style, for example
querySelector(":dir(ltr)") must work even when everything is display: none.

[kojii, 2016-05-20 07:52:07]

On 2016/05/19 at 21:29:17, esprehn wrote:
> On 2016/05/19 at 10:39:12, kojii wrote:
> > On 2016/05/19 at 09:48:31, rune wrote:
> > > <style>div:dir(rtl) span {}</style>
> > > <div dir="auto"><span></span>...<span></span></div>
> > > 
> > > you'd resolve dir="auto" for the div as many times as you have span
descendants. A per-style-resolve cache for dir="auto" elements could a good
idea.
> > 
> > At this moment, my preference is, use CSS direction property to resolve
:dir(), assuming it's possible because of my understanding that matchAllRules()
comes after CSS direction property was resolved.
> > 
> 
> That's backwards, matchAllRules() is *before* any style is resolved. Also we
need to be able to match rules when there's no style, for example
querySelector(":dir(ltr)") must work even when everything is display: none.

mistook, s/matchAllRules/matchAuthorRules/. We resolve all presentational
attributes and dir=auto before that IIUC:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

but true for display:none.

So we should prefer always traverse, assuming rune@'s case should not exist in
the real world?

[Timothy Loh, 2016-05-25 07:41:48]

Since BlinkOn is coming up, maybe it'd be convenient to just discuss this in
person (assuming we're all going) unless there's any pressing need to get this
in sooner?

[rune, 2016-05-25 07:43:14]

On 2016/05/25 07:41:48, Timothy Loh wrote:
> Since BlinkOn is coming up, maybe it'd be convenient to just discuss this in
> person (assuming we're all going) unless there's any pressing need to get this
> in sooner?

I'll be there.

[kojii, 2016-05-25 08:27:48]

On 2016/05/25 at 07:43:14, rune wrote:
> On 2016/05/25 07:41:48, Timothy Loh wrote:
> > Since BlinkOn is coming up, maybe it'd be convenient to just discuss this in
> > person (assuming we're all going) unless there's any pressing need to get
this
> > in sooner?
> 
> I'll be there.

I'll be there too, only on the first day though.

[esprehn, 2016-06-01 05:08:27]

On 2016/05/25 at 08:27:48, kojii wrote:
> On 2016/05/25 at 07:43:14, rune wrote:
> > On 2016/05/25 07:41:48, Timothy Loh wrote:
> > > Since BlinkOn is coming up, maybe it'd be convenient to just discuss this
in
> > > person (assuming we're all going) unless there's any pressing need to get
this
> > > in sooner?
> > 
> > I'll be there.
> 
> I'll be there too, only on the first day though.

sgtm.

[Timothy Loh, 2017-01-12 06:04:39]

Description was changed from

==========
[WIP] Implement :dir()
==========

to

==========
[WIP] Implement :dir()

Design notes:
https://docs.google.com/document/d/15lOJZqOKT5X_9HDs1lqN1nvcttsCyd9OrSv50yW6d...
==========