Bug 216257 – REGRESSION (r257839): clickpay.com - password placeholder text cannot be replaced

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

<rdar://problem/68150686>

[Wenson Hsieh]

Created attachment 408283 [details]
Patch

[Wenson Hsieh]

Created attachment 408299 [details]
Patch

[Wenson Hsieh]

Created attachment 408331 [details]
For EWS

[Wenson Hsieh]

Created attachment 408336 [details]
For EWS

[Wenson Hsieh]

Created attachment 408341 [details]
For EWS

[Wenson Hsieh]

Created attachment 408345 [details]
For EWS

[Antti Koivisto]

Comment on attachment 408345 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=408345&action=review

> Source/WebCore/dom/Element.cpp:3422
> +    auto* style = hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag) ? nullptr : existingComputedStyle();
>      if (!style)
>          style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);

Shouldn't we clear the flag in resolveComputedStyle?

[Wenson Hsieh]

Comment on attachment 408345 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=408345&action=review

>> Source/WebCore/dom/Element.cpp:3422
>>          style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);
> 
> Shouldn't we clear the flag in resolveComputedStyle?

Whoops — that’s right, we should clear the flag when we cache the computed style in that function. Will update the patch to do so!

[Wenson Hsieh]

After chatting with Antti, we both think this new flag should exist in ElementRareData alongside the computedStyle it is about, too.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #9)
> After chatting with Antti, we both think this new flag should exist in
> ElementRareData alongside the computedStyle it is about, too.

…but as it turns out, doing so would require us to increase the size of ElementRareData, so just having the flag here is more optimal :/

[Wenson Hsieh]

Created attachment 408462 [details]
Patch for landing

[EWS]

Committed r266887: <https://trac.webkit.org/changeset/266887>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408462 [details].

[Darin Adler]

Comment on attachment 408462 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=408462&action=review

> Source/WebCore/dom/Element.cpp:1976
> +    if (hasRareData() && elementRareData()->computedStyle())
> +        setNodeFlag(NodeFlag::IsComputedStyleInvalidFlag);

Do we really need to check computedStyle for null? Isn’t it harmless to set this flag if there is no computed style?

[Wenson Hsieh]

Comment on attachment 408462 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=408462&action=review

>> Source/WebCore/dom/Element.cpp:1976
>> +        setNodeFlag(NodeFlag::IsComputedStyleInvalidFlag);
> 
> Do we really need to check computedStyle for null? Isn’t it harmless to set this flag if there is no computed style?

Good question! The thought here was that if we hadn’t already set a computed style at this point, then we shouldn’t need to mark as invalid. However, after adding code to clear the flag when setting computed style below, I agree this check is not very useful. The only other codepath that calls setComputedStyle is in `Element::storeDisplayContentsStyle`, so we should be able to remove this if we make sure to clear the flag out there as well.

I’ll do this in a followup.

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 408486 [details]
Address comment

[EWS]

Committed r266899: <https://trac.webkit.org/changeset/266899>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408486 [details].