Created attachment 353319 [details] Testcase Selecting the pseudo element ::first-line produces an unnatural visual appearance of selecting the text as if the pseudo element ::first-line did not exist. We should make selecting ::first-line feel natural by inheriting style from the first line. You can observe this issue using the attached test case. Open the attached test case. Select the first line of text. You should see yellow text with a yellow underline on top of a blue background. But you get magenta text with a yellow underline on top of a blue background.
Created attachment 353321 [details] Patch and layout test
Oops! Ignore iOS EWS. Selection is painted by UIKit. So, will skip the text on iOS.
Comment on attachment 353321 [details] Patch and layout test Attachment 353321 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9772327 New failing tests: fast/css/selection-inherits-first-line-styles.html
Created attachment 353330 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 353436 [details] Patch and layout tests
If the patch for bug #141774 lands before the patch for this bug. Then I will update the patch for this bug to include the following diff: [[ diff --git a/Source/WebCore/rendering/InlineTextBox.cpp b/Source/WebCore/rendering/InlineTextBox.cpp index fbc1143ab65..ce22d4cd8a5 100644 --- a/Source/WebCore/rendering/InlineTextBox.cpp +++ b/Source/WebCore/rendering/InlineTextBox.cpp @@ -784,7 +784,7 @@ auto InlineTextBox::resolveStyleForMarkedText(const MarkedText& markedText, cons style.overlappedPseudoStyles.add(PseudoId::Selection); style.textDecorationsInEffect = ([&] { - if (auto pseudoStyle = renderer().selectionPseudoStyle()) + if (auto pseudoStyle = renderer().selectionPseudoStyle(&lineStyle())) return pseudoStyle->textDecorationsInEffect(); return lineStyle().textDecorationsInEffect(); })(); ]]
Comment on attachment 353436 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=353436&action=review > Source/WebCore/ChangeLog:18 > + Makes the pseudo element ::selection inherit the styles from pseudo element ::first-line. > + This changes the visual appearance when selecting the first line in a paragraph to more > + closely match intuition. That is, the painted selection looks more analogous to the effect > + of using a highlighter to highlight the line. > + > + Currently the pseudo element ::selection inherits the styles of the originating element. > + However selecting the pseudo element ::first-line produces the unnatural visual appearance > + of selecting the text as if it did not have pseudo element ::first-line. Instead the > + pseudo element ::selection should inherit the styles of the ::first-line, originating element > + (in that order) so a to produce a more aesthetically pleasing result that more closely > + matches intuition. Are there some real world examples where this is a problem? The fact is that our (and everyone elses) ::first-line is rather buggy (in more fundamental ways than selection) and there are major differences between browser engines. Perhaps as a result the feature is not used much. While improving quality is generally nice I'm not sure it is worth any increased complexity in this particular case. > Source/WebCore/rendering/RenderElement.h:79 > + Color selectionColor(CSSPropertyID, const RenderStyle* parentStyle = nullptr) const; > + std::unique_ptr<RenderStyle> selectionPseudoStyle(const RenderStyle* parentStyle = nullptr) const; > > // Obtains the selection colors that should be used when painting a selection. > - Color selectionBackgroundColor() const; > - Color selectionForegroundColor() const; > - Color selectionEmphasisMarkColor() const; > + Color selectionBackgroundColor(const RenderStyle* parentStyle = nullptr) const; > + Color selectionForegroundColor(const RenderStyle* parentStyle = nullptr) const; > + Color selectionEmphasisMarkColor(const RenderStyle* parentStyle = nullptr) const; How would a caller know when they need to provide the 'parentStyle' and when it is ok to leave it out? (here and many other places where you add this)
Comment on attachment 353436 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=353436&action=review >> Source/WebCore/ChangeLog:18 >> + matches intuition. > > Are there some real world examples where this is a problem? The fact is that our (and everyone elses) ::first-line is rather buggy (in more fundamental ways than selection) and there are major differences between browser engines. Perhaps as a result the feature is not used much. While improving quality is generally nice I'm not sure it is worth any increased complexity in this particular case. I do not know off the top of my head any real world examples. We could try searching <https://httparchive.org> (I will look when I have a moment). This patch is a step towards supporting correct inheritance in a future with more overlapping pseudo elements, such as spelling-error. I also like that this patch makes selection pretty with respect to a ::first-line style. >> Source/WebCore/rendering/RenderElement.h:79 >> + Color selectionEmphasisMarkColor(const RenderStyle* parentStyle = nullptr) const; > > How would a caller know when they need to provide the 'parentStyle' and when it is ok to leave it out? (here and many other places where you add this) I am not happy about this too. I am open to suggestions on an improved design for querying the selection colors of some marked text (the styled substring of the inline text box that may or may not have an associated document marker). The purpose of the additional argument is to let the caller pass the style of the marked text (though for now we only track the style of the line). This problem is not unique to these functions and we have historically solved this by passing a isFirstLine/firstLine boolean to various functions so that they inherit from the appropriate style. I chose to have these functions take a const RenderStyle* as opposed to a boolean so as to make the solution more generic in preparation for a future where we have more overlapping pseudo elements than just first-line (e.g. spelling-error).
Comment on attachment 353436 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=353436&action=review >>> Source/WebCore/ChangeLog:18 >>> + matches intuition. >> >> Are there some real world examples where this is a problem? The fact is that our (and everyone elses) ::first-line is rather buggy (in more fundamental ways than selection) and there are major differences between browser engines. Perhaps as a result the feature is not used much. While improving quality is generally nice I'm not sure it is worth any increased complexity in this particular case. > > I do not know off the top of my head any real world examples. We could try searching <https://httparchive.org> (I will look when I have a moment). This patch is a step towards supporting correct inheritance in a future with more overlapping pseudo elements, such as spelling-error. I also like that this patch makes selection pretty with respect to a ::first-line style. This patch isn't that complicated. We shouldn't need evidence to accept obvious bug fixes. The correct behavior is very obvious and our current behavior is clearly wrong. Dan handed us this patch on a silver platter, and there's no reason to reject it.
Comment on attachment 353436 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=353436&action=review > Source/WebCore/ChangeLog:11 > + of using a highlighter to highlight the line. So the inheritance chain is selection -> first line -> element's style >>> Source/WebCore/rendering/RenderElement.h:79 >>> + Color selectionEmphasisMarkColor(const RenderStyle* parentStyle = nullptr) const; >> >> How would a caller know when they need to provide the 'parentStyle' and when it is ok to leave it out? (here and many other places where you add this) > > I am not happy about this too. I am open to suggestions on an improved design for querying the selection colors of some marked text (the styled substring of the inline text box that may or may not have an associated document marker). The purpose of the additional argument is to let the caller pass the style of the marked text (though for now we only track the style of the line). This problem is not unique to these functions and we have historically solved this by passing a isFirstLine/firstLine boolean to various functions so that they inherit from the appropriate style. I chose to have these functions take a const RenderStyle* as opposed to a boolean so as to make the solution more generic in preparation for a future where we have more overlapping pseudo elements than just first-line (e.g. spelling-error). I'd love to see your next patch that passes in other values for this RenderStyle.
Comment on attachment 353436 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=353436&action=review >>>> Source/WebCore/rendering/RenderElement.h:79 >>>> + Color selectionEmphasisMarkColor(const RenderStyle* parentStyle = nullptr) const; >>> >>> How would a caller know when they need to provide the 'parentStyle' and when it is ok to leave it out? (here and many other places where you add this) >> >> I am not happy about this too. I am open to suggestions on an improved design for querying the selection colors of some marked text (the styled substring of the inline text box that may or may not have an associated document marker). The purpose of the additional argument is to let the caller pass the style of the marked text (though for now we only track the style of the line). This problem is not unique to these functions and we have historically solved this by passing a isFirstLine/firstLine boolean to various functions so that they inherit from the appropriate style. I chose to have these functions take a const RenderStyle* as opposed to a boolean so as to make the solution more generic in preparation for a future where we have more overlapping pseudo elements than just first-line (e.g. spelling-error). > > I'd love to see your next patch that passes in other values for this RenderStyle. Yeah, I agree with Antti that it isn't a great - I hope there's a way for the renderer's methods to just know which style is the correct style, instead of passing styles as arguments everywhere. > LayoutTests/platform/ios/TestExpectations:122 > +fast/css/selection-inherits-first-line-styles.html [ WontFix ] heehee
(In reply to Myles C. Maxfield from comment #9) > Comment on attachment 353436 [details] > Patch and layout tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353436&action=review > > >>> Source/WebCore/ChangeLog:18 > >>> + matches intuition. > >> > >> Are there some real world examples where this is a problem? The fact is that our (and everyone elses) ::first-line is rather buggy (in more fundamental ways than selection) and there are major differences between browser engines. Perhaps as a result the feature is not used much. While improving quality is generally nice I'm not sure it is worth any increased complexity in this particular case. > > > > I do not know off the top of my head any real world examples. We could try searching <https://httparchive.org> (I will look when I have a moment). This patch is a step towards supporting correct inheritance in a future with more overlapping pseudo elements, such as spelling-error. I also like that this patch makes selection pretty with respect to a ::first-line style. > > This patch isn't that complicated. We shouldn't need evidence to accept > obvious bug fixes. The correct behavior is very obvious and our current > behavior is clearly wrong. Dan handed us this patch on a silver platter, and > there's no reason to reject it. While the patch is not complicated at all, it is clearly not in the right direction in terms of maintainability. I bet the earlier patches of this first line style handling were not complicated either and yet we've ended up with a bunch of seemingly random functions taking bools and parent style pointers to handle inheritance. While this feature looks visually super pleasant, I don't see the rush in landing it. -I am not sure what an alternative approach would be like either though :( On the other hand, while this is clearly a longstanding design issue, we never made any effort (that I know of) to fix it, so it might not be that important/intrusive at all.
> This patch isn't that complicated. I think we have different view of what is 'complicated'. To me with an uncomplicated patch, a question like I asked [1] would have an uncomplicated answer. [1] "How would a caller know when they need to provide the 'parentStyle' and when it is ok to leave it out?"
(In reply to Antti Koivisto from comment #13) > > This patch isn't that complicated. > > I think we have different view of what is 'complicated'. To me with an > uncomplicated patch, a question like I asked [1] would have an uncomplicated > answer. > > [1] "How would a caller know when they need to provide the 'parentStyle' and > when it is ok to leave it out?" I haven't worked it out the design fully, yet, and I am open to ideas. A non-null "parentStyle" is only ever passed when painting inline boxes. More specifically, the "parentStyle" is either the style of the line (::first-line or the originating element's style) OR is the style of the marked text on the line. One thought I had was to represent the first line using a MarkedText object instead of booleans or a parentStyle. Towards this I would add a new enumerator MarkedText::FirstLine, extract InlineTextBox:: StyledMarkedText to be WebCore::StyledMarkedText (or flatten it into MarkedText) and teach StyledMarkedText to hold a RenderStyle, revert the parentStyle argument in the RenderElement/RenderText::selection* functions and introduce new selection*ForMarkedText() variants that take a StyledMarkedText&. Have the current RenderElement/RenderText::selection*() functions turn around and call selection*ForMarkedText() with a StyledMarkedText for the "unmarked text" (MarkedText::Unmarked) - the originating element's base style. I would also rename the RenderText::selection*() functions to RenderText::selection*ForMarkedText() as we always need to compute the selection style with respect to the marked text. (That is, it never makes sense to compute selection with respect to the originating element's style for text). Regardless, I do not see the need to defer landing this patch to fix up the design. This work seems better suited in a separate bug and can be done incrementally.