[webkit-reviews] review denied: [Bug 80794] :first-line pseudo selector ignoring words created from :before : [Attachment 167255] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 80794: :first-line pseudo selector ignoring words created from :before
https://bugs.webkit.org/show_bug.cgi?id=80794

Attachment 167255: Patch
https://bugs.webkit.org/attachment.cgi?id=167255&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167255&action=review


> Source/WebCore/ChangeLog:30
> +	   helper function getFirstLineStyle().

getFirstLineStyle() => firstLineStyleForCachedUncachedType()

>
LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:13

> +p:first-line {
> +    color: green;
> +}
> +p:first-letter {
> +    color: blue;
> +}	

It bothers me that we are using :first-line and :first-letter in this expected
results file given that this patch fixes an issue with generated
content/pseudo-elements. Although this expected results file uses existing
machinery, including machinery we preserved in this patch, it is preferred that
we construct the expected results without the use of the :first-letter and
:first-line pseudo-elements so as to further differentiate the code path taken
to generate the actual and expected results of the test, respectively. Can we
come up with a way to generate these expected results without using :first-line
and :first-letter? Maybe construct the contents of the HTML p elements (below)
using stylized <span>s?

>
LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:14

> +</style>

Please remove the tab character at the end of the previous line.

>
LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:19

> +    <br/>

Nit: No need to explicitly terminate the br element in an HTML5 document. We
can write this as <br>.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:25
> +}	

Please remove the tab character at the end of this line.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:31
> +    <br/>

Nit: No need to explicitly terminate the br element in an HTML5 document. We
can write this as <br>.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:37
> +    <p><span id="before"> of this paragraph should be displayed in green
color.</span></p>

This is invalid markup by uniqueness of the HTML attribute id,
<http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the-
id-attribute>. That is, the HTML attribute id for the <span> must be unique,
but it isn't. The first occurrence of this id was on line 34.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:43
> +    <p><span id="after">Display </span></p>

This is invalid markup by uniqueness of the HTML attribute id,
<http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the-
id-attribute>. That is, the HTML attribute id for the <span> must be unique,
but it isn't. The first occurrence of this id was on line 40.