Closed
Bug 1364280
Opened 8 years ago
Closed 8 years ago
stylo: dynamic dir attribute changes don't seem to cause restyling of descendants properly
Categories
(Core :: CSS Parsing and Computation, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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.
|
Assignee | |
Comment 1•8 years ago
|
||
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 hidden (mozreview-request) |
|
||
Comment 3•8 years ago
|
||
| mozreview-review | ||
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+
|
|
Assignee | |
Comment 4•8 years ago
|
||
> 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.
| Comment hidden (mozreview-request) |
|
||
Comment 6•8 years ago
|
||
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
|
||
Comment 8•8 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
|
||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
You need to log in
before you can comment on or make changes to this bug.
Description
•