Closed Bug 1364280 Opened 7 years ago Closed 7 years ago

stylo: dynamic dir attribute changes don't seem to cause restyling of descendants properly

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1364280. Properly restyle on dir attr changes.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
Pretty simple testcase:

  <style>
    div { color: red; }
    div:dir(ltr) { color: green }
  </style>
  <body dir="rtl" onload="document.body.dir = 'ltr'">
    <div>
      This should be green.
    </div>
  </body>

The text is not green in stylo.
Depends on: 1364335
Three more, slightly more complicated, testcases that should all end up passing:

  <style>
    span { color: red; }
    div:dir(rtl) + span { color: green; }
  </style>
  <body dir="rtl" onload="document.querySelector('[dir=auto]').setAttribute('dir', '')">
    <div dir="auto"></div>
    <span>This should be green</span>
  </body>

  <style>
    span { color: green; }
    div:not(:dir(rtl)) + span { color: red; }
  </style>
  <body dir="rtl" onload="document.querySelector('[dir=auto]').setAttribute('dir', '')">
    <div dir="auto"></div>
    <span>This should be green</span>
  </body>

  <style>
    span { color: green; }
    div[dir="auto"]:dir(ltr) + span { color: red; }
  </style>
  <body dir="rtl" onload="document.querySelector('[dir=auto]').setAttribute('dir', '')">
    <div dir="auto"></div>
    <span>This should be green</span>
  </body>
Comment on attachment 8867346 [details]
Bug 1364280.  Properly restyle on dir attr changes.

https://reviewboard.mozilla.org/r/138876/#review142256

r=me with nits, thanks for fixing this! :)

::: servo/components/style/restyle_hints.rs:318
(Diff revision 1)
>          }
>      }
>  }
>  
> +#[cfg(feature = "gecko")]
> +fn dir_selector_to_state(s: &Box<[u16]>) -> ElementState {

nit: This can use `&[u16]` directly, and save an indirection level.

::: servo/components/style/restyle_hints.rs:321
(Diff revision 1)
>  
> +#[cfg(feature = "gecko")]
> +fn dir_selector_to_state(s: &Box<[u16]>) -> ElementState {
> +    // Jump through some hoops to deal with our Box<[u16]> thing.
> +    let ltr_literal = [b'l' as u16, b't' as u16, b'r' as u16, 0];
> +    let rtl_literal = [b'r' as u16, b't' as u16, b'l' as u16, 0];

nit: Not 100% sure, but probably you can make these const:

```
const LTR: [u16; 4] = [b'l' as u16, ...];
const RTL: [u16; 4] = ...;
```

::: servo/components/style/restyle_hints.rs:323
(Diff revision 1)
> +fn dir_selector_to_state(s: &Box<[u16]>) -> ElementState {
> +    // Jump through some hoops to deal with our Box<[u16]> thing.
> +    let ltr_literal = [b'l' as u16, b't' as u16, b'r' as u16, 0];
> +    let rtl_literal = [b'r' as u16, b't' as u16, b'l' as u16, 0];
> +    if ltr_literal == **s {
> +        return IN_LTR_STATE;

nit: you can either drop `else`s after return, or drop `return` and semicolons, whatever you prefer.
Attachment #8867346 - Flags: review?(emilio+bugs) → review+
> nit: This can use `&[u16]` directly, and save an indirection level.

Nice, done.

> nit: Not 100% sure, but probably you can make these const:

Indeed, I can.  Done.

> nit: you can either drop `else`s after return, or drop `return` and semicolons, whatever you prefer.

Good point; this got rearranged a few times and then I didn't self-review very well.  Dropped the returns.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8ba288c0fd94 -d 78c489199cd8: rebasing 395837:8ba288c0fd94 "Bug 1364280.  Properly restyle on dir attr changes.  r=emilio" (tip)
merging layout/reftests/bugs/reftest.list
warning: conflicts while merging layout/reftests/bugs/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb7c67e56a8e
test expectations update, now that we properly restyle on dir attr changes.  r=emilio
https://hg.mozilla.org/mozilla-central/rev/fb7c67e56a8e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → bzbarsky
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: