Summary: | Crash in RenderElement::selectionPseudoStyle with detail element set to display: contents | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Doug Kelly <dougk> | ||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, rniwa, simon.fraser, webkit-bug-importer, zalan | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | macOS 10.15 | ||||||||||||
Attachments: |
|
Description
Doug Kelly
2020-01-23 15:16:25 PST
Created attachment 388598 [details]
Patch
Comment on attachment 388598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388598&action=review r=me but I have to admit the logic of the change is not obvious to me > Source/WebCore/rendering/RenderElement.cpp:1419 > + while (currentElement && currentElement->hasDisplayContents()) > + currentElement = currentElement->parentElement(); It’s not obvious *why* this is the right thing to do. So we need a comment. Why are we skipping over any element that "has display contents"? (In reply to Darin Adler from comment #2) > Comment on attachment 388598 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388598&action=review > > r=me but I have to admit the logic of the change is not obvious to me > > > Source/WebCore/rendering/RenderElement.cpp:1419 > > + while (currentElement && currentElement->hasDisplayContents()) > > + currentElement = currentElement->parentElement(); > > It’s not obvious *why* this is the right thing to do. So we need a comment. > Why are we skipping over any element that "has display contents"? Ryosuke? You were the one that mentioned that we needed to find the first parent without "display: contents" set -- but it ultimately has something to do with the next line, where we need a valid renderer. Is there something special about "display: contents" that would need this? Created attachment 388888 [details]
Patch
I managed to speak with Ryosuke, and he confirmed that if an element is set to "display: contents" it will not have a renderer. Hopefully the improved comment clarifies this; I must admit that I'm still not terribly familiar with its behaviors. Comment on attachment 388888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388888&action=review > Source/WebCore/rendering/RenderElement.cpp:1419 > + // If an Element hasDisplayContents(), it will not have a renderer. > + // Instead, we must walk up the tree to find a parent element with a valid renderer. This comment more or less repeats what the code says. I'd try giving more of a why comment like this: // When an element has display: contents, this element doesn't have a renderer // and its children will render as children of the parent element. There is no need to say that we're looking for the first element without display: contents since that's pretty self explanatory from the code. (In reply to Ryosuke Niwa from comment #6) > Comment on attachment 388888 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388888&action=review > > > Source/WebCore/rendering/RenderElement.cpp:1419 > > + // If an Element hasDisplayContents(), it will not have a renderer. > > + // Instead, we must walk up the tree to find a parent element with a valid renderer. > > This comment more or less repeats what the code says. > I'd try giving more of a why comment like this: > // When an element has display: contents, this element doesn't have a > renderer > // and its children will render as children of the parent element. > > There is no need to say that we're looking for the first element without > display: contents > since that's pretty self explanatory from the code. Agreed; I was trying to think of the best way to word it without adding anything which might be misleading. This sounds much better. :) Created attachment 388897 [details]
Patch
Comment on attachment 388897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388897&action=review > Source/WebCore/rendering/RenderElement.cpp:1417 > + Element* currentElement = element()->shadowHost(); auto* please Created attachment 389262 [details]
Patch
The commit-queue encountered the following flaky tests while processing attachment 389262 [details]:
The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 389262 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch. Comment on attachment 389262 [details] Patch Clearing flags on attachment: 389262 Committed r255448: <https://trac.webkit.org/changeset/255448> All reviewed patches have been landed. Closing bug. |