In bug 135293, we found that we can simplify current SelectorChecker's :visited handling code. It makes CSS JIT :visited code simpler
For non-style cases (querySelector etc) we should just filter out :visited rules after parsing as they can never match. This should be trivial using SelectorChecker::determineLinkMatchType().
Hi Antti and Benjamin, After our discussion, I investigated why the nested link and adjacents are handled specially. And maybe I've figure out why it is! At first, I think these handling don't protect from any attacks. This is because all attacks are already protected by linkMatchType. I'll describe my understanding of WebKit :visited handling. And I'll suggest how to simplify the current implementation. Since I'm a new commer to this :visited handling, if I miss something, please point it :) WebKit :visited rendering is following. 1. SelectorChecker::determineLinkMatchType clasifies selector into 4 types. MatchLink = 1:(01), MatchVisited = 2:(10), MatchAll = 3:(11) and neither = 0:(00). 2. Perform selector matching on the element. (SelectorChecker) 3. When the selector is matched, apply the properties to the element's style. In this time, elements have 3 css values per property. cssValue[0] => neither cssValue[1] => link cssValue[2] => visited Basically, we apply cssValue[linkMatchType] = newValue; But when the type is MatchAll, we apply cssValue[0..3] = newValue; 4. (maybe) When rendering, we calculate linkState of the element, which is clasified into 3 types, NotInsideLink, InsideUnvisitedLink and InsideVisitedLink. We propagate this value from the parent to the child. If the element is link, we override this value with the element link's linkState. otherwise the linkState is inherited from the parent. 5. At last, by using linkState, we choose the cssValue and render the element with it. But only white listed properties use cssValue[1 or 2]. Such as background-color. OK, so let's describe the important questions. Q1: Is it protected from any attacks? I think yes. For example, getComputedStyle always reads cssValue[0]. And cssValue[0] is not modified with :visited/:link selectors. So the element always returns the non-link-dependent value. And when rendering the element, reading cssValue[1 or 2] is only allowed for white listed properties, it doens't have effect on layouts (for example, "height" cannot be used). As the result, there's no way to read :visited dependent values programmably. Q2: So why adjacent selectors are disabled? (:visited) This is not for security issues. It's already solved. In the phase 4, we calculate the linkState from the parent. So the linkState doesn't consider the adjacent linkStates. For example, provided the selector ":visited + span" <p> <a href="visited"> <a href="link"></a> <span>target</span> </a> </p> In this HTML, if we don't disable adjacent :visited, <span> would be matched to the selector in the phase 2. This is because SelectorChecker doesn't see the link's status. And after that, in the phase 4, since the parent <a> is :visited, the linkState of the <span> becomes :visited. So span is matched and the style is applied to this element. Calculating the depending adjacent's link state is too high cost, the current semantics is taken I think. Q3: So why the nested links are disabled? (:visited) This is not for security issues. It's already solved. In the phase 4, we calculate the linkState from the parent and overrides it with the current element's linkState. (In StyleResolver) As the result, the linkState of the element is the linkState of the closest link. If the link is nested, the closest link's linkState only becomes effective. This is why introducing the semantics, "the closest link's :visited is only effective". Suggestion: How to simplify it? Considering these things, I think we can simplify SelectorChecker's isLink check for :visited and handles it in the StyleResolver side. Currently, in phase 4, we always overrides linkState with the current element's linkState, like if (element->isLink) { linkState = lookupLinkState(element); } Instead, by changing it to if (element->isLink) { previousLinkState = linkState; if (previousLinkState is InsideUnvisitedLink or InsideVisitedLink) linkState = NotInsideLinkForced } I think we can handle nested link check in StyleResolver side. What do you think about this?
(In reply to comment #2) Errata. > 5. > At last, by using linkState, we choose the cssValue and render the element with it. > But only white listed properties use cssValue[1 or 2]. Such as background-color. Constructing regular / visited property values from these values in StyleResolver::CachedProperties::Property::apply. > Q1: Is it protected from any attacks? > > I think yes. For example, getComputedStyle always reads cssValue[0]. And cssValue[0] is not modified with :visited/:link selectors. > So the element always returns the non-link-dependent value. > And when rendering the element, reading cssValue[1 or 2] is only allowed for white listed properties, it doens't have effect on layouts (for example, "height" cannot be used). > As the result, there's no way to read :visited dependent values programmably. getComputedStyle will lookup with allowVisitedStyle = false. So CSSComputedStyleDeclaration doesn't use visitedDependentColor. These visitedDependentColor is stored in the style separately. And they are set by DeprecatedStyleBuilder's handlers.
(In reply to comment #2) > Suggestion: How to simplify it? > Oooooops! I've missed the pattern. Considering the all patterns, we cannot remove this isLink() check... Here's 2 examples, <!DOCTYPE HTML> <html lang="en"> <head> <meta charset="UTF-8"> <title></title> <style type="text/css"> .ok span { color: green; } .ok:visited span { color: red; } </style> </head> <body> <p><a href="file:///prpr" class="ok" id="out"></a></p> <script type="text/javascript"> // https://apple.com should be visited. var a = document.createElement('a');a.href="https://apple.com/";a.innerHTML = '<span>hello</span>'; document.getElementById('out').appendChild(a); </script> </body> </html> In this HTML, we constructed the tree, <p> <a href="file:///prpr" class="ok" id="out"> <a href="https://apple.com/"> <span>hello</span> </a> </a> </p> On Firefox, link becomes green, but on WebKit, link becomes red. Since .ok should not be matched to <a href="file:///prpr" class="ok" id="out">, I think it should be green. And the example 2, <!DOCTYPE HTML> <html lang="en"> <head> <meta charset="UTF-8"> <title></title> <style type="text/css"> .ok span { color: green; } :visited span { color: red; } </style> </head> <body> <p><a href="file:///prpr" class="ok" id="out"></a></p> <script type="text/javascript"> var a = document.createElement('a');a.href="https://apple.com/";a.innerHTML = '<span>hello</span>'; document.getElementById('out').appendChild(a); </script> </body> </html> In this HTML, we constructed the same tree, but the selector ":visited span" is different. In this example, both WebKit / Firefox show the red link and it is correct. But my proposal breaks it.
(In reply to comment #1) > For non-style cases (querySelector etc) we should just filter out :visited rules after parsing as they can never match. This should be trivial using SelectorChecker::determineLinkMatchType(). Thank you for your advice! It looks very nice. I think ":-webkit-any(:visited, others)" causes problems. Currently, :-webkit-any + :visited behavior is, + linkMatchType becomes MatchAll (so, :visited information is discarded) + in selector matching, "VisitedMatchDisabled" has effect on :visited inside :-webkit-any. To make it consistent, I suggest that treating :visited and :link inside :-webkit-any as :-webkit-any-link. (Since the linkMatchType is MatchAll currently) Do you think about this plan?
The case of nested links is incredibly annoying. I agree with you, SelectorChecker is broken in the example you gave. For the JIT, I don't think we should maintain a live state during all matching. This would add a lot of complexity for an uncommon case. What do you think of: 1) On entry, push the element on the stack. 2) Reserve stack space for one more pointer. Let's call it "alpha". 3) Perform the selector matching. Every time ":visited" is encountered, push the current element register to "alpha". 4) In case of successful matching. Do a tree walking from the start point saved in (1). If any isLink() is encountered before "alpha", fail. Otherwise succeed. (In reply to comment #5) > (In reply to comment #1) > > For non-style cases (querySelector etc) we should just filter out :visited rules after parsing as they can never match. This should be trivial using SelectorChecker::determineLinkMatchType(). > > Thank you for your advice! It looks very nice. > I think ":-webkit-any(:visited, others)" causes problems. > Currently, :-webkit-any + :visited behavior is, > > + linkMatchType becomes MatchAll (so, :visited information is discarded) > + in selector matching, "VisitedMatchDisabled" has effect on :visited inside :-webkit-any. > > To make it consistent, I suggest that treating :visited and :link inside :-webkit-any as :-webkit-any-link. (Since the linkMatchType is MatchAll currently) > Do you think about this plan? Yep, that sounds like a good idea. Combining :-webkit-any() and :visited is not worth the headache :)
(In reply to comment #6) > The case of nested links is incredibly annoying. > > I agree with you, SelectorChecker is broken in the example you gave. > > For the JIT, I don't think we should maintain a live state during all matching. This would add a lot of complexity for an uncommon case. > > What do you think of: > 1) On entry, push the element on the stack. > 2) Reserve stack space for one more pointer. Let's call it "alpha". > 3) Perform the selector matching. Every time ":visited" is encountered, push the current element register to "alpha". > 4) In case of successful matching. Do a tree walking from the start point saved in (1). If any isLink() is encountered before "alpha", fail. Otherwise succeed. Sounds nice! I think it works correctly for :visited :) One corner case is :not(:link). :not(:link) behaves if (element->isLink() && VisitedMatchDisabled) return false; return true; So if we remove VisitedMatchDisabled check at traversing phase, this predicate becomes, return true; So, in phase (4), we cannot determine the matching is succeeded or not. For example, provided ".ok > :not(:link) .target" and the tree <body.ok><div.ok><a><a><span.target> Since :not(:link) always matches at phase (3), matching succeeds and the result becomes, <body.ok> <div.ok> => .ok> <a> => :not(:link) <a> <span> => .target But it failes at (4). However this matching should succeed like that, <body.ok> => .ok> <div.ok> => :not(:link) <a> <a> <span> => .target Possibly, since :not(:link) is very rare case, I think we need not to pay cost to speed up :not(:link) case. So I suggest treating it with a special function call. For example, when :not(:link) is executed in phase (3), we call such a function to calculate (4) at (3) dynamically, bool matchesNotLink(Element* target, Element* start, bool* cachedDisabled) { if (!target->isLink()) return true; if (*cachedDisabled) return false; for (Element* cursor = start; cursor != target; cursor = cursor->parentElement()) { if (cursor->isLink()) { *cachedDisabled = true; return false; } } return true; } What do you think about this?
Created attachment 237046 [details] Patch
Comment on attachment 237046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237046&action=review WIP > Source/WebCore/css/SelectorChecker.cpp:-131 > -SelectorChecker::SelectorChecker(Document& document, Mode mode) Move mode from SelectorChecker to Context. It makes context layout more similar to CSS JIT Context. > Source/WebCore/css/SelectorChecker.cpp:172 > +static SelectorChecker::SelectorCheckingContext walkToParentElement(const SelectorChecker::SelectorCheckingContext& context) Not to forget to check current element `isLink` when walking to the parent node, extracted it to this function. > Source/WebCore/css/SelectorChecker.cpp:237 > + if (relation != CSSSelector::Descendant && relation != CSSSelector::Child) Don't check `isLink` here. > Source/WebCore/css/SelectorChecker.cpp:248 > + for (; nextContext.element; nextContext = walkToParentElement(nextContext)) { Use `walkToParentElement` in descendant traversing. > Source/WebCore/css/SelectorChecker.cpp:705 > + return eleemnt->isLink(); When `:-webkit-any(:visited)` case, treat it as `:-webkit-any(:-any-link)`. It keeps consistent between selector matching behavior and linkMatchType. > Source/WebCore/css/SelectorChecker.h:-58 > - SelectorCheckingContext(const CSSSelector* selector, Element* element, VisitedMatchType visitedMatchType) Hide VisitedMatchType from public API.
Created attachment 237059 [details] Patch Patch: 1st version
Comment on attachment 237059 [details] Patch This is the first version of this patch. In this patch, we don't apply aggressive changes because :visited filtering out version still requires some dirty code... I'll attach the second version later.
Created attachment 237065 [details] Patch Patch: 2st version
Comment on attachment 237065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237065&action=review Added comments on 2nd version. In this version, because of :not(:link), we cannot clean up adjacentFound flag... That's why I attached the simple 1st patch separately. > Source/WebCore/css/ElementRuleCollector.cpp:340 > + if (ruleData.visitedNeverMatches()) We can filter out some cases. But some cases cannot be filter out. So still `adjacentFound` exists... > Source/WebCore/css/SelectorChecker.cpp:239 > + if (relation != CSSSelector::Descendant && relation != CSSSelector::Child) Still need to track adjacentFound... > Source/WebCore/css/SelectorChecker.cpp:506 > + if (subContext.selector->pseudoClassType() == CSSSelector::PseudoClassLink && subContext.resolvingMode != Mode::QueryingRules && !subContext.adjacentFound && isFirstFoundLink(subContext.element, subContext.matchingStartElement)) `adjacentFound` is used here. > Source/WebCore/css/SelectorChecker.cpp:712 > + RELEASE_ASSERT_WITH_MESSAGE(!context.adjacentFound, "Adjacent case never matches with :visited and it is filtered out before the matching."); Here, since QueryingRules are filtered out above and :visited with adjacent cases are also filtered out, we can guarantee this assertion. > Source/WebCore/css/SelectorChecker.cpp:938 > + neverMatches = true; Find neverMatches case.
Comment on attachment 237059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237059&action=review > Source/WebCore/css/SelectorChecker.cpp:172 > +static SelectorChecker::SelectorCheckingContext walkToParentElement(const SelectorChecker::SelectorCheckingContext& context) The name walkToParentElement() is a bit too generic in this context. What about? checkingContextForParent() > Source/WebCore/css/SelectorChecker.cpp:703 > + // Inside functional pseudo class except for :not, :visited is treated as :-any-link since linkMatchType becomes MatchAll. > + if (context.inFunctionalPseudoClass) We should make sure there are tests for that. > LayoutTests/fast/history/nested-visited-test-override.html:5 > +jsTestIsAsync = true; You should also call description() to explain what you are testing with this test. > LayoutTests/fast/history/visited-inside-any.html:23 > + oneStyle = internals.computedStyleIncludingVisitedInfo(document.getElementById("one").firstChild); > + twoStyle = internals.computedStyleIncludingVisitedInfo(document.getElementById("two")); > + shouldBecomeEqualToString("oneStyle.color", "rgb(0, 128, 0)", finish); > + shouldBecomeEqualToString("twoStyle.color", "rgb(0, 128, 0)", finish); Hum, seeing this test, it looks like treating :-webkit-any(:visited) as :any-link could be counter intuitive for web developers. Maybe :visited should never match instead. What do you think?
Comment on attachment 237065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237065&action=review >> Source/WebCore/css/ElementRuleCollector.cpp:340 >> + if (ruleData.visitedNeverMatches()) > > We can filter out some cases. But some cases cannot be filter out. So still `adjacentFound` exists... I don't think filtering ahead is worth the trouble. The pseudo class :visited is quite uncommon, even more so on complex selectors. > Source/WebCore/css/SelectorChecker.cpp:131 > +static inline bool isFirstFoundLink(const Element* lastVisitedMatched, const Element* start) The name could be super verbose here, something like lastVisistedElementIsFirstLink() or something like that.
Comment on attachment 237059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237059&action=review Thank you for your detailed review! >> Source/WebCore/css/SelectorChecker.cpp:172 >> +static SelectorChecker::SelectorCheckingContext walkToParentElement(const SelectorChecker::SelectorCheckingContext& context) > > The name walkToParentElement() is a bit too generic in this context. > > What about? checkingContextForParent() `checkingContextForParent` looks quite nice. >> Source/WebCore/css/SelectorChecker.cpp:703 >> + if (context.inFunctionalPseudoClass) > > We should make sure there are tests for that. Right. I'll add :not(:visited) and ::cue test cases. >> LayoutTests/fast/history/nested-visited-test-override.html:5 >> +jsTestIsAsync = true; > > You should also call description() to explain what you are testing with this test. Thank you! I'll add it. >> LayoutTests/fast/history/visited-inside-any.html:23 >> + shouldBecomeEqualToString("twoStyle.color", "rgb(0, 128, 0)", finish); > > Hum, seeing this test, it looks like treating :-webkit-any(:visited) as :any-link could be counter intuitive for web developers. > > Maybe :visited should never match instead. What do you think? Agree with you. In the case of :-webkit-any(:visited, ...), we cannot determine appropriate linkMatchType for this deterministically, so currently this linkMatchType becomes MatchAll. But it is counter intuitive. So simply disabling :-webkit-any(:visited) matching sounds nice. And I think we also need to consider about :-webkit-any(:link). Currently it is also treated as :-webkit-any(:any-link) since linkMatchType is MatchAll and :link only checks isLink. What do you think about this? I think disabling :-webkit-any(:link) too provides consistent behavior.
Comment on attachment 237065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237065&action=review >>> Source/WebCore/css/ElementRuleCollector.cpp:340 >>> + if (ruleData.visitedNeverMatches()) >> >> We can filter out some cases. But some cases cannot be filter out. So still `adjacentFound` exists... > > I don't think filtering ahead is worth the trouble. The pseudo class :visited is quite uncommon, even more so on complex selectors. Make sense. Checking adjacentFound in :visited check is more straightforward and easy to understand the code flow. In the CSS JIT phase, we can statically check this and make the selector CannotMatchAnything :) >> Source/WebCore/css/SelectorChecker.cpp:131 >> +static inline bool isFirstFoundLink(const Element* lastVisitedMatched, const Element* start) > > The name could be super verbose here, something like > lastVisistedElementIsFirstLink() > or something like that. lastVisistedElementIsFirstLink is quite expressive. I'll take `lastVisistedElementIsFirstLink` name when implementing CSS JIT.
I cleared the flags to let you update. I let you decide which version you want to go with, they are both reasonable approaches. You can always change your mind later and refactor the code as needed, your test ensure we avoid regressions.
Created attachment 237356 [details] Patch
Comment on attachment 237356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237356&action=review > Source/WebCore/css/SelectorChecker.cpp:707 > + return false; This changes :-webkit-any(:visited). But :-webkit-any(:link) behavior is not changed. What do you think? Since still we need to consider what is the best behavior about :-webkit-any and :visited, :link, in this time, I don't change :-webkit-any(:link) behavior.
Comment on attachment 237356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237356&action=review Some comments but it is minor. I trust you with the update, but if you want I can also review it. > Source/WebCore/ChangeLog:9 > + Fix :-webkit-any(:visited) consistency in linkMatchType and actual selector matching. > + And fix disabling :visited matching when the first link comes. It would be good to have a more descriptive changelog. A lot of important information is on the bug report itself. It is a good idea to also repeat that in the changelog. With such information, someone can figure out what your patch does and why you are taking this approach. Changelog are especially useful when trying to find the reasoning behind a change that was very far in the past. Sometime we have to investigate issue related to code that is over 10 years old. >> Source/WebCore/css/SelectorChecker.cpp:707 >> + return false; > > This changes :-webkit-any(:visited). But :-webkit-any(:link) behavior is not changed. > What do you think? > Since still we need to consider what is the best behavior about :-webkit-any and :visited, :link, in this time, I don't change :-webkit-any(:link) behavior. I am okay with this as a temporary measure. I think you should also disable :link for consistency (:link would always match inside a functional pseudo class). We need big changes for :not() and :-webkit-any()/:matches(), that's gonna be painful :( > Source/WebCore/css/SelectorChecker.h:58 > - SelectorCheckingContext(const CSSSelector* selector, Element* element, VisitedMatchType visitedMatchType) > + SelectorCheckingContext(const CSSSelector* selector, Element* element, Mode resolvingMode) Can you move "VisitedMatchType" inside SelectorChecker.cpp now? Or is it still referenced somewhere? > LayoutTests/ChangeLog:9 > + Fix :-webkit-any(:visited) consistency in linkMatchType and actual selector matching. > + And fix disabling :visited matching when the first link comes. You don't need to repeat the changelog for test. Usually here you describe the tests when they are not obvious. > LayoutTests/ChangeLog:16 > + * fast/history/nested-visited-test-override-expected.txt: Added. > + * fast/history/nested-visited-test-override.html: Added. > + * fast/history/visited-inside-any-expected.txt: Added. > + * fast/history/visited-inside-any.html: Added. > + * fast/history/visited-inside-not-expected.txt: Added. > + * fast/history/visited-inside-not.html: Added. It is very good to have this. I would duplicate all of that for :link since they are opposite. > LayoutTests/fast/history/nested-visited-test-override.html:12 > + description(":visited is only effective when the target is first encountered link element."); We usually put the description() call at the beginning of the <script>, outside the test function.
Comment on attachment 237356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237356&action=review >> Source/WebCore/ChangeLog:9 >> + And fix disabling :visited matching when the first link comes. > > It would be good to have a more descriptive changelog. > > A lot of important information is on the bug report itself. It is a good idea to also repeat that in the changelog. With such information, someone can figure out what your patch does and why you are taking this approach. > > Changelog are especially useful when trying to find the reasoning behind a change that was very far in the past. Sometime we have to investigate issue related to code that is over 10 years old. Thank you! I'll add more detailed comments here. >>> Source/WebCore/css/SelectorChecker.cpp:707 >>> + return false; >> >> This changes :-webkit-any(:visited). But :-webkit-any(:link) behavior is not changed. >> What do you think? >> Since still we need to consider what is the best behavior about :-webkit-any and :visited, :link, in this time, I don't change :-webkit-any(:link) behavior. > > I am okay with this as a temporary measure. I think you should also disable :link for consistency (:link would always match inside a functional pseudo class). > > We need big changes for :not() and :-webkit-any()/:matches(), that's gonna be painful :( Ah, currently, since :link doesn't see visitedMatchType in SelectorChecker, :-webkit-any(:link) behaves :-webkit-any(:any-link). And I think it is consistent behavior. (It matches `(:link would always match inside a functional pseudo class).`, I think :-webkit-any(:link) should check isLink at least.) To ensure that, I've added more tests about :link :) >> Source/WebCore/css/SelectorChecker.h:58 >> + SelectorCheckingContext(const CSSSelector* selector, Element* element, Mode resolvingMode) > > Can you move "VisitedMatchType" inside SelectorChecker.cpp now? Or is it still referenced somewhere? Since "VisitedMatchType" is also used in `SelectorCheckerFastPath.h`, it requires more cleanups. I'll do it in https://bugs.webkit.org/show_bug.cgi?id=135255 :) >> LayoutTests/ChangeLog:16 >> + * fast/history/visited-inside-not.html: Added. > > It is very good to have this. I would duplicate all of that for :link since they are opposite. Sounds nice. I'll add :link tests. >> LayoutTests/fast/history/nested-visited-test-override.html:12 >> + description(":visited is only effective when the target is first encountered link element."); > > We usually put the description() call at the beginning of the <script>, outside the test function. Thanks! I'll move it.
Committed r173138: <http://trac.webkit.org/changeset/173138>