[webkit-reviews] review granted: [Bug 191040] Make selection inherit from first-line : [Attachment 353436] Patch and layout tests

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 191040: Make selection inherit from first-line
https://bugs.webkit.org/show_bug.cgi?id=191040

Attachment 353436: Patch and layout tests

https://bugs.webkit.org/attachment.cgi?id=353436&action=review




--- Comment #11 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 353436
  --> https://bugs.webkit.org/attachment.cgi?id=353436
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