Bug 52195 - REGRESSION(r71934) Can't type in search edit field on skin-one.com
Summary: REGRESSION(r71934) Can't type in search edit field on skin-one.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P1 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords: InRadar
Depends on:
Blocks: 46015
  Show dependency treegraph
 
Reported: 2011-01-10 21:51 PST by Adele Peterson
Modified: 2011-01-11 12:45 PST (History)
3 users (show)

See Also:


Attachments
Patch (16.21 KB, patch)
2011-01-11 11:10 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
With better ChangeLog descriptions. (16.85 KB, patch)
2011-01-11 11:13 PST, Dimitri Glazkov (Google)
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2011-01-10 21:51:19 PST
<rdar://problem/8807518>

It looks like this site is trying to prevent copying of resources by disabling selection:

document.onselectstart=new Function ("return false")

You can reproduce the problem with this markup:
<script>document.onselectstart=new Function("return false")</script>
<input>

Or with this url:
data:text/html,%3Cscript%3Edocument.onselectstart=new%20Function%20(%22return%20false%22)%3C/script%3E%3Cinput%3E

I think before this change, that wouldn't have prevented setting selection in a text field. 

Firefox doesn't appear to allow preventing selection start from this event at all.  We should see what IE does.

Comment 1 Dimitri Glazkov (Google) 2011-01-11 07:42:29 PST
My booger. Will fix.
Comment 2 Dimitri Glazkov (Google) 2011-01-11 07:42:54 PST
(In reply to comment #0)
> <rdar://problem/8807518>
> 
> It looks like this site is trying to prevent copying of resources by disabling selection:
> 
> document.onselectstart=new Function ("return false")
> 
> You can reproduce the problem with this markup:
> <script>document.onselectstart=new Function("return false")</script>
> <input>
> 
> Or with this url:
> data:text/html,%3Cscript%3Edocument.onselectstart=new%20Function%20(%22return%20false%22)%3C/script%3E%3Cinput%3E
> 
> I think before this change, that wouldn't have prevented setting selection in a text field. 
> 
> Firefox doesn't appear to allow preventing selection start from this event at all.  We should see what IE does.
> 

Thanks for an excellent reduction, Adele!
Comment 3 Dimitri Glazkov (Google) 2011-01-11 10:02:50 PST
IE behaves differently from pre-regression change. Here's the breakdown:

1) Pre-regression, WebKit behavior was to allow selection inside text input and textareas.

2) Post-regression, you can't even type into the field.

3) IE doesn't allow selection anywhere, though you can type into it all you want.

Should I:

a) restore original behavior, which seems a bit wrong (at least compatibility-wise)

b) make WebKit behave like IE?
Comment 4 Dimitri Glazkov (Google) 2011-01-11 10:28:01 PST
Ew, even selectstart always returns true, keyboard selection still works in WebKit. I think this yak is too big to shave at the moment. I'll just unregress the behavior and leave the compatibility issue alone.
Comment 5 Dimitri Glazkov (Google) 2011-01-11 11:10:54 PST
Created attachment 78556 [details]
Patch
Comment 6 Dimitri Glazkov (Google) 2011-01-11 11:13:43 PST
Created attachment 78559 [details]
With better ChangeLog descriptions.
Comment 7 Eric Seidel (no email) 2011-01-11 11:39:55 PST
Comment on attachment 78559 [details]
With better ChangeLog descriptions.

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

OK.

> Source/WebCore/dom/Node.cpp:2628
> +    getEventAncestors(ancestors, originalTarget.get(), event->isMutationEvent() || event->type() == eventNames().selectstartEvent ? StayInsideShadowDOM : RetargetEvent);

Should this check be its own function?  Explaining why mutation events and selection start events are in thsi set?
Comment 8 Dimitri Glazkov (Google) 2011-01-11 11:50:45 PST
(In reply to comment #7)
> (From update of attachment 78559 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78559&action=review
> 
> OK.
> 
> > Source/WebCore/dom/Node.cpp:2628
> > +    getEventAncestors(ancestors, originalTarget.get(), event->isMutationEvent() || event->type() == eventNames().selectstartEvent ? StayInsideShadowDOM : RetargetEvent);
> 
> Should this check be its own function?  Explaining why mutation events and selection start events are in thsi set?

Great idea, will add a helper function with comments.
Comment 9 Dimitri Glazkov (Google) 2011-01-11 12:45:04 PST
Committed r75536: <http://trac.webkit.org/changeset/75536>