Bug 49868 – REGRESSION (r71934): Main search field at Apple tech specs does not accept typing

• Component: DOM
• Status: RESOLVED FIXED

To reproduce, navigate to the URL, focus the search field next to “Search Tech Specs” and type.

<rdar://problem/8692047>

[Alexey Proskuryakov]

The only relevant commit in this range is the shadow DOM-aware event targeting one.

[Mark Rowe (bdash)]

Confirmed via bisection that <http://trac.webkit.org/changeset/71934> is to blame.

This also breaks the search field in the web inspector.

[Dave Hyatt]

Just a guess, but focus/blur events that remain inside the shadow nodes of the same enclosing explicit element should not retarget.  The same is also true for mouseover and mouseout events that remain inside the object.  If those are now propagating out, that would cause odd behaviors.

See:

http://www.w3.org/TR/xbl/#event2

and then also

sections 6.10 and 6.11.

[Pavel Feldman]

Is anyone looking at this? It does break inspector's search.

[Dimitri Glazkov (Google)]

Fixing..

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> Is anyone looking at this? It does break inspector's search.

FWIW, tech specs behavior is slightly this is different from inspector's search. You can still search in Inspector by clicking on the loupe.

I've been tracking through the code, have some leads.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> Just a guess, but focus/blur events that remain inside the shadow nodes of the same enclosing explicit element should not retarget.  The same is also true for mouseover and mouseout events that remain inside the object.  If those are now propagating out, that would cause odd behaviors.
> 
> See:
> 
> http://www.w3.org/TR/xbl/#event2
> 
> and then also
> 
> sections 6.10 and 6.11.

Yup, I didn't yet implement that. However, all of the shadow DOM elements today are explicitly marked as not focusable. I think the issue is with the style recalc not being triggered when focusing somehow. If you go in Inspector and modify width, the problem goes away.

[Dimitri Glazkov (Google)]

Created attachment 74812 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> Created an attachment (id=74812) [details]
> Patch

This corrects the issue with the site reported. The problem still persists with the Inspector, but it's a totally different issue, related to manipulating (removing, actually) selection while inside of the focus event. I'll file a separate bug.

[Pavel Feldman]

> This corrects the issue with the site reported. The problem still persists with the Inspector, but it's a totally different issue, related to manipulating (removing, actually) selection while inside of the focus event. I'll file a separate bug.

Any idea what regressed it? Is there a bug already?

[Dimitri Glazkov (Google)]

(In reply to comment #11)
> > This corrects the issue with the site reported. The problem still persists with the Inspector, but it's a totally different issue, related to manipulating (removing, actually) selection while inside of the focus event. I'll file a separate bug.
> 
> Any idea what regressed it? Is there a bug already?

No yet, I'll file and provide reduction and fix after the Thanksgiving break. It regressed with the same revision.

[Darin Adler]

Comment on attachment 74812 [details]
Patch

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

I wonder if some of the code in RenderBlock::hasLineIfEmpty can now be deleted.

> LayoutTests/fast/forms/disabled-search-input.html:59
>  \ No newline at end of file

We normally put those end-of-file newlines in WebKit source files.

> WebCore/rendering/TextControlInnerElements.cpp:54
> +    virtual bool hasLineIfEmpty() const { return true; }

I slightly prefer that such overrides be private rather than public. It’s typically a programming error if someone calls the function on a RenderTextControlInnerBlock* directly, and it’s nice to have this caught at compile time. And the privacy has no effect on overriding.

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> (From update of attachment 74812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74812&action=review
> 
> I wonder if some of the code in RenderBlock::hasLineIfEmpty can now be deleted.

Not yet. The spin buttons still use it :( But soon, soon, I shall make this all be not so crappy! :)

> 
> > LayoutTests/fast/forms/disabled-search-input.html:59
> >  \ No newline at end of file
> 
> We normally put those end-of-file newlines in WebKit source files.
> 
> > WebCore/rendering/TextControlInnerElements.cpp:54
> > +    virtual bool hasLineIfEmpty() const { return true; }
> 
> I slightly prefer that such overrides be private rather than public. It’s typically a programming error if someone calls the function on a RenderTextControlInnerBlock* directly, and it’s nice to have this caught at compile time. And the privacy has no effect on overriding.

Sounds great! Wil do.

[Dimitri Glazkov (Google)]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 74812 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74812&action=review
> > 
> > I wonder if some of the code in RenderBlock::hasLineIfEmpty can now be deleted.
> 
> Not yet. The spin buttons still use it :( But soon, soon, I shall make this all be not so crappy! :)

I think I can remove one check though. I'll do that.

> 
> > 
> > > LayoutTests/fast/forms/disabled-search-input.html:59
> > >  \ No newline at end of file
> > 
> > We normally put those end-of-file newlines in WebKit source files.
> > 
> > > WebCore/rendering/TextControlInnerElements.cpp:54
> > > +    virtual bool hasLineIfEmpty() const { return true; }
> > 
> > I slightly prefer that such overrides be private rather than public. It’s typically a programming error if someone calls the function on a RenderTextControlInnerBlock* directly, and it’s nice to have this caught at compile time. And the privacy has no effect on overriding.
> 
> Sounds great! Wil do.

[Dimitri Glazkov (Google)]

Committed r72807: <http://trac.webkit.org/changeset/72807>