Closed
Bug 1474959
Opened 6 years ago
Closed 6 years ago
thread '<unnamed>' panicked at 'assertion failed: self.writing_mode.map_or(true, |wm| wm == writing_mode)', servo/components/style/rule_cache.rs:40:9
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla63
| Tracking | Status | |
|---|---|---|
| firefox-esr52 | --- | unaffected |
| firefox-esr60 | --- | unaffected |
| firefox61 | --- | unaffected |
| firefox62 | --- | unaffected |
| firefox63 | --- | fixed |
People
(Reporter: tsmith, Assigned: emilio)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker])
Attachments
(8 files)
|
183 bytes,
text/html
|
Details | |
|
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
Details | |
|
59 bytes,
text/x-review-board-request
|
Details | |
|
59 bytes,
text/x-review-board-request
|
Details | |
|
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
|
35.32 KB,
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
|
11.24 KB,
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
Reduced with m-c: BuildID=20180711094246 SourceStamp=aff060ad3204234adae2d59b3776207c6687ebfc The fuzzers have been hitting this issue frequently since yesterday afternoon. thread '<unnamed>' panicked at 'assertion failed: self.writing_mode.map_or(true, |wm| wm == writing_mode)', servo/components/style/rule_cache.rs:40:9 0|0|firefox|mozalloc_abort|hg:hg.mozilla.org/mozilla-central:memory/mozalloc/mozalloc_abort.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|34|0x5 0|1|firefox|abort|hg:hg.mozilla.org/mozilla-central:memory/mozalloc/mozalloc_abort.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|81|0x5 0|2|libxul.so|_ZN11panic_abort18__rust_start_panic5abort17hfb98714efe360e0fE|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:915ac660268eeff1cee8f6e59670f19b501a143e|61|0x6 0|3|libxul.so|__rust_start_panic|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:915ac660268eeff1cee8f6e59670f19b501a143e|56|0x6 0|4|libxul.so|rust_panic|git:github.com/rust-lang/rust:src/libstd/panicking.rs:915ac660268eeff1cee8f6e59670f19b501a143e|559|0x5 0|5|libxul.so|_ZN3std9panicking20rust_panic_with_hook17h608586f043d70222E|git:github.com/rust-lang/rust:src/libstd/panicking.rs:915ac660268eeff1cee8f6e59670f19b501a143e|531|0xb 0|6|libxul.so|_ZN3std9panicking11begin_panic17hd4c46bd0d5679291E|git:github.com/rust-lang/rust:src/libstd/panicking.rs:915ac660268eeff1cee8f6e59670f19b501a143e|445|0x20 0|7|libxul.so|_ZN5style10properties9longhands25border_inline_start_width16cascade_property17h4f63a2f0df9efb44E|hg:hg.mozilla.org/mozilla-central:servo/components/style/rule_cache.rs:aff060ad3204234adae2d59b3776207c6687ebfc|40|0x18 0|8|libxul.so|_ZN5style10properties7cascade17h4e09fd1ac4d72296E|s3:gecko-generated-sources:8d1b5b5f5fefe1eef0302fcdbb5cf0f017338c0a0486ab37aae77633b33f6f923f5ba176c3f5d79090ca356d3d10861290d1f5f4b74dae52a16bb723ad53b6ce/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-4661bea949fb2a68/out/properties.rs:|82143|0x12 0|9|libxul.so|_ZN5style7stylist7Stylist25cascade_style_and_visited17h555512092bbfc5c8E|hg:hg.mozilla.org/mozilla-central:servo/components/style/stylist.rs:aff060ad3204234adae2d59b3776207c6687ebfc|904|0x49 0|10|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$25cascade_style_and_visited17h202bbdaacd0e3f63E|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|306|0x28 0|11|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$21cascade_primary_style17hd572f170bb70d550E|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|216|0x17 0|12|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$35cascade_styles_with_default_parents28_$u7b$$u7b$closure$u7d$$u7d$17hf6ff545a8166e9cfE|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|336|0x8 0|13|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$35cascade_styles_with_default_parents17h684fe6583d24215dE|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|102|0xb 0|14|libxul.so|_ZN5style9traversal13compute_style17hccde0c3c8a613b66E|hg:hg.mozilla.org/mozilla-central:servo/components/style/traversal.rs:aff060ad3204234adae2d59b3776207c6687ebfc|663|0x5 0|15|libxul.so|_ZN168_$LT$style..gecko..traversal..RecalcStyleOnly$LT$$u27$recalc$GT$$u20$as$u20$style..traversal..DomTraversal$LT$style..gecko..wrapper..GeckoElement$LT$$u27$le$GT$$GT$$GT$16process_preorder17h652c998a879b70d7E|hg:hg.mozilla.org/mozilla-central:servo/components/style/traversal.rs:aff060ad3204234adae2d59b3776207c6687ebfc|434|0x15 0|16|libxul.so|_ZN5style6driver12traverse_dom17h00cd87b45169009eE|hg:hg.mozilla.org/mozilla-central:servo/components/style/driver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|111|0x1b 0|17|libxul.so|_ZN10geckoservo4glue16traverse_subtree17h6effab5464b8672dE|hg:hg.mozilla.org/mozilla-central:servo/ports/geckolib/glue.rs:aff060ad3204234adae2d59b3776207c6687ebfc|293|0xe 0|18|libxul.so|Servo_TraverseSubtree|hg:hg.mozilla.org/mozilla-central:servo/ports/geckolib/glue.rs:aff060ad3204234adae2d59b3776207c6687ebfc|351|0x11 0|19|libxul.so|mozilla::ServoStyleSet::StyleDocument(mozilla::ServoTraversalFlags)|hg:hg.mozilla.org/mozilla-central:layout/style/ServoStyleSet.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|996|0x1d 0|20|libxul.so|mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags)|hg:hg.mozilla.org/mozilla-central:layout/base/RestyleManager.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|2965|0x12 0|21|libxul.so|mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|4289|0x19 0|22|libxul.so|nsRefreshDriver::Tick(long, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1934|0x5 0|23|libxul.so|mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|303|0xb 0|24|libxul.so|mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|322|0xf 0|25|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|767|0x5 0|26|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|579|0xc 0|27|libxul.so|mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&)|hg:hg.mozilla.org/mozilla-central:layout/ipc/VsyncChild.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|68|0x9 0|28|libxul.so|mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&)|s3:gecko-generated-sources:0c7cf777c2ff93c34ff1546f677320cb1229427e6947e87c6fa76720f9b9c5b6a4a4d036521ed9a643f4fa5e10a57d8748e2532d47fce8282aa653340c0c00ff/ipc/ipdl/PVsyncChild.cpp:|167|0xc 0|29|libxul.so|mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|2134|0x6 0|30|libxul.so|mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|2064|0xb 0|31|libxul.so|mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1910|0xb 0|32|libxul.so|mozilla::ipc::MessageChannel::MessageTask::Run()|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1943|0xc 0|33|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1051|0x15 0|34|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|519|0x11 0|35|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|97|0xa 0|36|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|325|0x17 0|37|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|318|0x8 0|38|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|158|0xd 0|39|libxul.so|XRE_RunAppShell()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|920|0x11 0|40|libxul.so|mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|269|0x5 0|41|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|325|0x17 0|42|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|318|0x8 0|43|libxul.so|XRE_InitChildProcess(int, char**, XREChildData const*)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|746|0x8 0|44|firefox|content_process_main(mozilla::Bootstrap*, int, char**)|hg:hg.mozilla.org/mozilla-central:ipc/contentproc/plugin-container.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|50|0x14 0|45|firefox|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|287|0x11 0|46|libc-2.23.so||||0x20830 0|47|firefox|MOZ_ReportAssertionFailure|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:aff060ad3204234adae2d59b3776207c6687ebfc|164|0x5
Flags: in-testsuite?
|
Reporter | |
Updated•6 years ago
|
status-firefox63:
--- → affected
|
||
Updated•6 years ago
|
Priority: -- → P3
|
||
Comment 2•6 years ago
|
||
This seems to a regression from bug 1472567 according to mozregression, but applying that patch locally doesn't seem to trigger this bug, so I reran the bisect, and but got the exactly same result. So probably there are some patches between my local tree and that bug broke some invariant that the patch relies on.
Blocks: 1472567
Keywords: regression
|
|
||
Comment 3•6 years ago
|
||
Oh wait, actually I can reproduce it locally...
|
Assignee | |
Updated•6 years ago
|
Flags: needinfo?(emilio)
|
|
Assignee | |
Comment 4•6 years ago
|
||
Ah, I see what's going on. We don't cascade writing-mode / etc for :visited, which means that the visited style ends up with a different WritingMode. I can fix this as soon as I find some time, if you don't get to it before, Xidorn. I still have a flight remaining though...
|
|
||
Comment 5•6 years ago
|
||
Oh, I see, good catch! I can fix it and you will get a patch to review when your flight finishes :)
Flags: needinfo?(emilio)
|
|
||
Updated•6 years ago
|
Assignee: nobody → xidorn+moz
|
|
||
Comment 6•6 years ago
|
||
It's actually a bit trickier than I initially thought. The problem here is that, we need the writing mode of the element to resolve the logical properties. However, we don't know the writing mode of the element for visited style because we compute visited style before the element style. We also cannot cascade writing mode properties for visited style because that would cause problems (both behavior difference on visual result and this assertion) when :visited has a different writing mode than the element style. I guess that means we would actually need to cascade the element style first then visited style.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6) > We also cannot cascade writing mode properties for visited style because > that would cause problems (both behavior difference on visual result and > this assertion) when :visited has a different writing mode than the element > style. I'm not convinced this is actually a problem: if the :visited style has a different writing-mode, it might swap which border-color is on which side... but that's not detectable in computed style, so why would it be a problem?
|
|
||
Comment 8•6 years ago
|
||
(In reply to David Baron :dbaron: ⌚UTC-7 from comment #7) > I'm not convinced this is actually a problem: if the :visited style has a > different writing-mode, it might swap which border-color is on which side... > but that's not detectable in computed style, so why would it be a problem? This is not a problem in terms of privacy because of the reason you said. But it is a problem that it doesn't match what the spec would want (because writing-mode etc. shouldn't apply via :visited), and internally it doesn't fix the assertion in this bug for all cases (because we would end up having a wrong rule condition when we start cascading the element style).
Where does the spec require not doing that?
|
|
||
Comment 10•6 years ago
|
||
The spec may not require not doing that, but it would be weird to apply color with a different writing mode than other stuff. And we need the correct behavior to appease the assertion anyway.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 16•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8991595 [details] Bug 1474959 part 1 - Have StyleBuilder return a UniqueArc. https://reviewboard.mozilla.org/r/256520/#review263376 ::: servo/components/style/properties/gecko.mako.rs:246 (Diff revision 1) > pseudo_ty: structs::CSSPseudoElementType, > pseudo_tag: *mut structs::nsAtom > - ) -> Arc<ComputedValues> { > + ) -> UniqueArc<ComputedValues> { > let arc = { > - let arc: Arc<ComputedValues> = Arc::new(uninitialized()); > + let arc: UniqueArc<ComputedValues> = UniqueArc::new(uninitialized()); > bindings::Gecko_ComputedStyle_Init(&arc.0 as *const _ as *mut _, nit: While you're here maybe fix the indentation?
Attachment #8991595 -
Flags: review?(emilio) → review+
|
|
||
Comment 17•6 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8991595 [details] Bug 1474959 part 1 - Have StyleBuilder return a UniqueArc. https://reviewboard.mozilla.org/r/256520/#review263376 > nit: While you're here maybe fix the indentation? This will be fixed in bug 1475229.
|
|
Assignee | |
Comment 18•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8991596 [details] Bug 1474959 part 2 - Set visited_style and visited bit after ComputedValues is built. https://reviewboard.mozilla.org/r/256522/#review263654 Behavior-wise looks fine, but it seems to me that it's a bit unfortunate that we need to care about visited explicitly in more cases... I'm leaving the r? flag for now because I want to take a closer look at this during the weekend or next monday. ::: servo/ports/geckolib/glue.rs:3088 (Diff revision 1) > - doc_data.stylist.device(), > + let mut style = StyleBuilder::for_inheritance( > + device, > Some(styles.primary()), > Some(pseudo), > - ).build().shareable() > + ).build(); > + style.inherit_visited(device, styles.primary()); It looks a bit unfortunate that for_inheritance no longer can do visited correctly and needs this... I want to take a closer look at this on monday to see if I manage to figure out a way to avoid this. It's also somewhat unfortunate that mBits is mutated while it's a `const` in C++... I think we may want to try to move the visited cascading to `cascade` / `apply_declarations`, and make a single `StyleBuilder` handle both visited and unvisited styles internally. That'd be cleaner I think.
|
|
||
Comment 19•6 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #18) > I think we may want to try to move the visited cascading to `cascade` / > `apply_declarations`, and make a single `StyleBuilder` handle both visited > and unvisited styles internally. `cascade` may work, while `apply_declarations` is already way long and complicated, and I don't think it's a good idea to put any extra level of logic into that...
|
|
Assignee | |
Comment 20•6 years ago
|
||
Sorry I couldn't get to this today... It's on my list for tomorrow :(
|
|
Assignee | |
Comment 21•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8991599 [details] Bug 1474959 part 5 - Add a visited reftest for different logical border color with different writing modes. https://reviewboard.mozilla.org/r/256528/#review264376 This looks great, thanks!
Attachment #8991599 -
Flags: review?(emilio) → review+
|
|
Assignee | |
Comment 22•6 years ago
|
||
I think this would be a bit easier to understand and maintain, wdyt Xidorn? The extra cascade_rules declaration is to avoid multiple monomorphizations of the apply_declarations function, but let me know if you don't think it's worth it. We can remove a style bit as well with this patch which I'll submit separately in a second.
Attachment #8992611 -
Flags: review?(xidorn+moz)
|
|
Assignee | |
Comment 23•6 years ago
|
||
It was only used by the rule cache, and we handled it badly at some places (though it didn't cause correctness issues). This patch takes advantage of the previous patch handling the visited in `apply_declarations` to avoid using the rule cache for visited style computation.
Attachment #8992612 -
Flags: review?(xidorn+moz)
|
||
Comment 24•6 years ago
|
||
(Leaving 62 as "?" due to https://bugzilla.mozilla.org/show_bug.cgi?id=1472567#c13)
status-firefox61:
--- → unaffected
status-firefox62:
--- → ?
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
|
|
||
Comment 25•6 years ago
|
||
Comment on attachment 8992611 [details] [diff] [review] Push visited style computation a bit further down. Review of attachment 8992611 [details] [diff] [review]: ----------------------------------------------------------------- OK, I guess it's the better way to fix this. I don't think it's really easier to understand but it's a smaller change anyway. r=me with the issues below addressed. ::: servo/components/style/properties/properties.mako.rs @@ +3702,5 @@ > where > E: TElement, > { > debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some()); > let empty = SmallBitVec::new(); It's not clear to me why do you move `property_restriction` into `rule_declarations_iterator` but not this one. I have a reverse feeling that this should probably go local into the iterating function, and restriction should probably be passed in instead. Actually it's not totally clear to me why you need to split this function at all. I don't see any other place using `rule_declarations_iterator`. Is there? @@ +3776,2 @@ > Some(parent_style) => { > + (parent_style, layout_parent_style.unwrap_or(parent_style)) I find you having both `layout_parent` and `layout_parent_style` around very confusing. If the non-Option then layout_parent_style (now layout_parent) is only used when we call StyleAdjuster, can we move the handling of that around StyleAdjuster::adjust instead? That should leave just one `layout_parent_style` around. ::: servo/components/style/stylist.rs @@ +848,3 @@ > // We need to compute visited values if we have visited rules or if our > // parent has visited values. > + let visited_rules = match inputs.visited_rules.as_ref() { This may be more concise: ``` let visited_rules = input.visited_rules.as_ref().or_else(|| { parent_style.and_then(|s| s.visited_style()).map(|_| { inputs.rules.as_ref().unwrap_or(self.rule_tree.root()) }) }); ```
Attachment #8992611 -
Flags: review?(xidorn+moz) → review+
|
|
||
Updated•6 years ago
|
Attachment #8991596 -
Flags: review?(emilio)
Attachment #8991597 -
Flags: review?(emilio)
Attachment #8991598 -
Flags: review?(emilio)
|
|
||
Comment 26•6 years ago
|
||
Comment on attachment 8992612 [details] [diff] [review] Remove IS_STYLE_IF_VISITED. Review of attachment 8992612 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/style/properties/computed_value_flags.rs @@ -56,5 @@ > /// Whether the child explicitly inherits any reset property. > const INHERITS_RESET_STYLE = 1 << 8; > > - /// A flag to mark a style which is a visited style. > - const IS_STYLE_IF_VISITED = 1 << 9; Rather than removing it, consider replacing it with something like > // const UNUSED_FLAG = 1 << 9; so that it is easier for people to see this empty slot when they are adding new flags.
Attachment #8992612 -
Flags: review?(xidorn+moz) → review+
|
||
Updated•6 years ago
|
|
|
Assignee | |
Comment 27•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #25) > Comment on attachment 8992611 [details] [diff] [review] > Push visited style computation a bit further down. > > Review of attachment 8992611 [details] [diff] [review]: > ----------------------------------------------------------------- > > OK, I guess it's the better way to fix this. I don't think it's really > easier to understand but it's a smaller change anyway. > > r=me with the issues below addressed. > > ::: servo/components/style/properties/properties.mako.rs > @@ +3702,5 @@ > > where > > E: TElement, > > { > > debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some()); > > let empty = SmallBitVec::new(); > > It's not clear to me why do you move `property_restriction` into > `rule_declarations_iterator` but not this one. I have a reverse feeling that > this should probably go local into the iterating function, and restriction > should probably be passed in instead. > > Actually it's not totally clear to me why you need to split this function at > all. I don't see any other place using `rule_declarations_iterator`. Is > there? I cannot because it doesn't outlive the function call otherwise. But you're right regarding that, I was going to use it in another place but then changed my mind. > @@ +3776,2 @@ > > Some(parent_style) => { > > + (parent_style, layout_parent_style.unwrap_or(parent_style)) > > I find you having both `layout_parent` and `layout_parent_style` around very > confusing. > > If the non-Option then layout_parent_style (now layout_parent) is only used > when we call StyleAdjuster, can we move the handling of that around > StyleAdjuster::adjust instead? That should leave just one > `layout_parent_style` around. Yeah, sounds fine. > ::: servo/components/style/stylist.rs > @@ +848,3 @@ > > // We need to compute visited values if we have visited rules or if our > > // parent has visited values. > > + let visited_rules = match inputs.visited_rules.as_ref() { > > This may be more concise: > ``` > let visited_rules = input.visited_rules.as_ref().or_else(|| { > parent_style.and_then(|s| s.visited_style()).map(|_| { > inputs.rules.as_ref().unwrap_or(self.rule_tree.root()) > }) > }); > ``` Hmm, not sure if I like it more or less... I'll give it a thought :)
|
|
Assignee | |
Comment 28•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26) > Comment on attachment 8992612 [details] [diff] [review] > Remove IS_STYLE_IF_VISITED. > > Review of attachment 8992612 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: servo/components/style/properties/computed_value_flags.rs > @@ -56,5 @@ > > /// Whether the child explicitly inherits any reset property. > > const INHERITS_RESET_STYLE = 1 << 8; > > > > - /// A flag to mark a style which is a visited style. > > - const IS_STYLE_IF_VISITED = 1 << 9; > > Rather than removing it, consider replacing it with something like > > // const UNUSED_FLAG = 1 << 9; > > so that it is easier for people to see this empty slot when they are adding > new flags. Instead I just renumbered the later servo flag.
|
||
Comment 29•6 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/mozilla-inbound/rev/454249841df9 Push visited style computation a bit further down. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/229e7ad57321 Remove IS_STYLE_IF_VISITED. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/56043453ebe6 Add a visited reftest for different logical border color with different writing modes. r=emilio
|
|
||
Updated•6 years ago
|
Assignee: xidorn+moz → emilio
|
||
Comment 30•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/454249841df9 https://hg.mozilla.org/mozilla-central/rev/229e7ad57321 https://hg.mozilla.org/mozilla-central/rev/56043453ebe6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
|
||
Comment 31•6 years ago
|
||
I'm not sure that ignoring writing-mode in :visited is compliant with the proposed spec (https://github.com/w3c/csswg-drafts/commit/4a4b81cf51524b5f74a877e8e7ce66881b5ac383) + 2. Otherwise, compute |el|’s style, + treating only its <a>relevant link</a> as matching '':visited'' + (and thus, not '':link''). + All other links must be treated as matching '':link''. + + 3. From the styling results, + record the values of the <dfn lt="allowed :visited property">allowed :visited properties</dfn>: + + * 'color' + * 'background-color' + * the 'border-color' sub-properties + * 'outline-color' + * 'column-rule-color' + * 'fill-color' + * 'stroke-color' + + These are |el|’s <dfn dfn lt=":visited style">:visited styles</dfn>. According to that, the writing mode should affect the logical-to-physical mapping in :visited. This is also what happens with CSS variables (https://github.com/w3c/csswg-drafts/issues/2263) like a { --foo: brown; } :visited { --foo: yellow; color: var(--foo); } The link is yellow in Firefox. It's not observable that --foo is yellow, but the effect on color is not ignored. For consistency, I think the same should happen with writing-mode and logical properties.
|
|
Assignee | |
Comment 32•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #31) > The link is yellow in Firefox. It's not observable that --foo is yellow, but > the effect on color is not ignored. For consistency, I think the same should > happen with writing-mode and logical properties. That means you'd need to track multiple different writing-modes, because the :link and :visited writing-modes could be different, if you allow writing-mode to affect :visited, or otherwise make the logical properties useless (by not honoring writing-mode and direction in the :visited rules), which is effectively what the spec says but I don't think it's the intended behavior.
|
|
||
Comment 33•6 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #32) > That means you'd need to track multiple different writing-modes Sure. I guess this is also what happens with custom properties. If I use getComputedStyle(a).getPropertyValue('--foo') I get " brown", but var(--foo) gets " yellow". I posted about this in https://github.com/w3c/csswg-drafts/issues/2844, maybe the CSSWG should discuss it.
|
|
||
Comment 34•6 years ago
|
||
Based on the spec text, we probably shouldn't be supporting logical border colors at all, since they are not subproperties of border-color at all currently.
|
|
||
Comment 35•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34) > Based on the spec text, we probably shouldn't be supporting logical border > colors at all, since they are not subproperties of border-color at all > currently. But physical and logical properties share computed values. So when you record the computed value of the physical border colors, this takes logical border colors into account.
You need to log in
before you can comment on or make changes to this bug.
Description
•