Closed Bug 1366144 Opened 6 years ago Closed 6 years ago

stylo: ::after pseudo element under a parent with transition didn't show up

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: shinglyu, Assigned: emilio)

References

Details

Attachments

(3 files, 2 obsolete files)

Stylo missing text screenshot
225.28 KB, image/png
Details
Gecko correct behavior screenshot
119.05 KB, image/png
Details
Bug 1366144: Test.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
STR:
* Open http://codepen.io/juliangarnier/pen/idhuG
* Observe the speed text under the "Earth" label

Expected: 
* There will be a speed number and a line "orbit velocity"

Actual:
* The text are missing

Notes:
* The text are ::after pseudo elements
  
  .set-speed dl.infos dd span::after {
      content: 'Orbit Velocity'; 
  }

* The ::after pseudo elements has a parent who has transitions:

  .infos, .icon {
      transition-duration: .8s;
      transition-timing-function: ease-in-out;
  }
Blocks: stylo
No longer blocks: stylo-reftest
Yeah, I don't think this is particularly related to transitions, here you got a test case that also fails locally for me:

<!DOCTYPE html>
<style>
  div::after { color: green }
  div.foo::after { content: "Bar" }
</style>
<div></div>
<script>
  onload = function() {
    document.querySelector('div').classList.add('foo');
  }
</script>
Assignee: nobody → emilio+bugs
cc heycam, since this may explain one of the regressions his incremental restyle optimizations have caused.
Wow, mozreview messed that up quite badly... Will rebase and see if re-pushing helps
Comment on attachment 8869454 [details]
Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content.

This patch won't work as written for various reasons, but mainly that the "matches_different_pseudos" logic is executed before cascading pseudos.

Since I prefer the first approach, and I don't think it's worth to tweak all that logic to hack around this, I'd prefer to go with only the first patch.

Also, https://treeherder.mozilla.org/#/jobs?repo=try&revision=490592187662557e393a677c2b159907592d4aa0 is a try run with this bug + bug 1364871. I'm surprised no new generated-content-related tests pass, so I'll add the test-case in my previous comment.
Attachment #8869454 - Flags: review?(cam)
Comment on attachment 8869435 [details]
Bug 1366144: Consider a display: none or empty-content ::before and ::after as not matching directly.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Comment on attachment 8869454 [details]
> Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if
> there's no generated content.
> 
> This patch won't work as written for various reasons, but mainly that the
> "matches_different_pseudos" logic is executed before cascading pseudos.
> 
> Since I prefer the first approach, and I don't think it's worth to tweak all
> that logic to hack around this, I'd prefer to go with only the first patch.
> 
> Also,
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=490592187662557e393a677c2b159907592d4aa0 is a try run
> with this bug + bug 1364871. I'm surprised no new generated-content-related
> tests pass, so I'll add the test-case in my previous comment.

Aaand this comment should be re. this attachment, sigh, sorry about that, but mozreview made them appear in the wrong order :(.
Attachment #8869435 - Flags: review?(cam)
Attachment #8869435 - Attachment is obsolete: true
Comment on attachment 8869454 [details]
Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content.

https://reviewboard.mozilla.org/r/141090/#review144820

::: servo/components/style/matching.rs:592
(Diff revision 3)
> +            // an ineffective content property...
> +            //
> +            // I think it's nice to handle it here instead for symmetry with how
> +            // we handle display: none elements, but the other approach may be
> +            // ok too?
> +            let is_unexisting_before_or_after =

Maybe "is_before_or_after_with_existing_style"?
Attachment #8869454 - Flags: review?(cam) → review+
Attachment #8869454 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c34ff92df5c3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.