Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#15990 closed defect (fixed)

setSelectable doesn't work in chrome, IE9+, opera

Reported by: morganm Owned by: Kenneth G. Franqueiro
Priority: blocker Milestone: 1.7.5
Component: HTML Version: 1.8.0
Keywords: ie9 ie10 Cc: ben hockey, Rawld Gill
Blocked By: Blocking:

Description

After calling setSelectable on a node, it is still selectable in Chrome.

I actually looked up what the problem is - setSelectable uses node.style.KhtmlUserSelect? = selectable ? "auto" : "none";

When I looked into it in chrome, there doesn't appear to be a .KhtmlUserSelect? property any more - it now appears to be .webkit-user-select.

See this page for more information: http://stackoverflow.com/questions/826782/css-rule-to-disable-text-selection-highlighting

In some very limited testing, simply setting the .webkit-user-select property seemed to work but there is a reference to -webkit-touch-callout: none as well, so maybe that should be looked into.

Attachments (2)

15990.diff (5.6 KB) - added by Kenneth G. Franqueiro 7 years ago.
(hopefully) final patch
15990_bill.patch (7.5 KB) - added by bill 7 years ago.
avoid exception in non-browser environments if if(!element)... ; also put the fallback handler second, just by convention

Download all attachments as: .zip

Change History (26)

comment:1 Changed 7 years ago by ben hockey

Cc: ben hockey added

comment:2 Changed 7 years ago by Kenneth G. Franqueiro

Milestone: tbd1.8.2
Owner: set to Kenneth G. Franqueiro
Priority: undecidedhigh
Status: newassigned

Given that this is browser-compat-related, I'd like to propose fixing this for 1.8.2. I ended up running across this while working on some code for dgrid, and ended up whipping up some code that we should be able to distill a more resilient version of this function from. I'll try to work it into an actual dom.js patch by the end of the week.

comment:3 Changed 7 years ago by bill

Component: GeneralHTML
Priority: highblocker

comment:4 Changed 7 years ago by Kenneth G. Franqueiro

I'm going to attach what I have so far; I've verified it in latest FF and Chrome, Opera 12, Safari 5.1 Windows, and IE6-10.

FWIW, the existing code wasn't working in IE9 either, because setting node.unselectable doesn't work in IE9 - but node.setAttribute("unselectable", ...) works everywhere that matters. And it naturally wasn't working in Opera because of the browser sniffs, even though the IE fallback should've worked just fine in it.

Unfortunately something's up, because the changes I've made cause builds to fail. Specifically, for some reason it seems to trip on the reference to element.style within the has test I added. I can't fathom why - first of all it's completely valid, and second of all it should be of absolutely no concern to the build process.

comment:5 Changed 7 years ago by bill

Summary: setSelectable doesn't work in chromesetSelectable doesn't work in chrome, IE9+, opera

No idea why your build is failing, but why don't you make has("css-user-select") return "userSelect" if that (non-vendor-specific) style attribute exists? So it will return "msUserSelect" for IE10, but "userSelect" for some possible future browser. That simplifies using the flag.

You could also replace the prefixes array w/an array of complete attribute names: ["userSelect", "msUserSelect", "OUserSelect", "MozUserSelect", "WebkitUserSelect", "KhtmlUserSelect"]. That simplifies the logic a bit, and I think it even reduces the overall size of the function.

comment:6 Changed 7 years ago by Kenneth G. Franqueiro

Those are good points, Bill; I solicited some more feedback this morning and got similar feedback from Forbes, who offered some suggestions that I'm incorporating in a new version of the patch.

The only thing out of your suggestions that hasn't been incorporated is the idea of expanding all the strings; while there'd be a negligible perf increase from not having to concatenate, I fail to see how this would be shorter when minified, since as it stands now, the variable names can be minified; strings cannot.

Ben concluded that this code is actually getting executed during builds (because the build runs dojo and this module is part of dojo base); Forbes also suggested an idea to avoid that, which I'll work on incorporating either tonight or tomorrow.

comment:7 Changed 7 years ago by Kenneth G. Franqueiro

I guess when the attachment replaced the original file it kept the original description; this is of course the second attempt, actually, but it's still failing builds until I get to take a whack at that later.

comment:8 Changed 7 years ago by bill

FYI, the idea of expanding all the strings is to remove the code handling "userSelect" as a special case:

if(typeof style.userSelect !== "undefined"){
	// Unlikely; user-select is non-standard, but might as well be future-proof...
	return "userSelect";
}

Avoiding the string concatenation is secondary.

Obviously the way you have it now isn't the end of the world, I just thought removing the if() statement was a slight improvement in code size (as well as code simplicity), especially taking into account gzip compression.

If you really like the string concatenation you can still reduce the code by doing something like:

var name = "userSelect";
...
do{
	if(style[name] !== undefined){
		return name;
	}
}while(i > 0 && name = prefixes[--i] + "UserSelect");

(PS: personally I would just use prefixes.length && name = prefixes.pop() + "UserSelect" as my loop condition but I see you are trying to do some performance optimization)

Anyway, the thing about the build system trying to execute the has() test is interesting. It seems to defeat the purpose of has() tests.

Last edited 7 years ago by bill (previous) (diff)

comment:9 Changed 7 years ago by Kenneth G. Franqueiro

I've fixed the build issue using Forbes' recommendation, and tried both of the approaches proposed above and compared their min+gz size. The expanded strings version was 5 bytes shorter than my version, and the prefix + do-while version was 6 bytes shorter than the expanded strings. Going to attach the patch with what I've arrived at. I'll commit tomorrow night if there are no more concerns.

Changed 7 years ago by Kenneth G. Franqueiro

Attachment: 15990.diff added

(hopefully) final patch

comment:10 Changed 7 years ago by bill

Cc: Rawld Gill added

I investigated the builder's undesirable behavior of loading dom.js. It's because we aren't specifying dojoConfig="async:true", so dojo.js is defaulting to it's back-compat behavior of loading a bunch of modules no one asked for explicitly. Unfortunately AFAICT it's hard to change that behavior. The builder has a --dojoConfig flag to load config info from a file, but it doesn't execute until too late.

Like I said on IRC though I'd prefer to work around that problem in a more standard way. Having setSelectable() redefine itself on the first call is a bit confusing and suboptimal for a webkit specific build that predefines "css-user-select". Plus it will break (admittedly unlikely) corner cases like other code setting up advice on setSelectable(). I'll attach a modified version of your patch w/what I'm thinking.

Changed 7 years ago by bill

Attachment: 15990_bill.patch added

avoid exception in non-browser environments if if(!element)... ; also put the fallback handler second, just by convention

comment:11 Changed 7 years ago by Kenneth G. Franqueiro

In [30097]:

Improve dom.setSelectable impl w/ feature detection and test page on 1.8 branch; refs #15990 !strict

comment:12 Changed 7 years ago by Kenneth G. Franqueiro

In [30098]:

Improve dom.setSelectable impl w/ feature detection and test page on trunk; refs #15990 !strict

comment:13 Changed 7 years ago by Kenneth G. Franqueiro

Milestone: 1.8.21.7.5

Updating ticket to reflect that this needs to get merged into 1.7 branch still; will do that later since the changeset couldn't be patched in automatically.

comment:14 Changed 7 years ago by bill

In [30108]:

Fix builder's handling of strings in staticHasFeatures, and add new "css-user-select" has() flag from [30098] to webkitMobile build, fixes #16421 and refs #15990 on trunk !strict

comment:15 Changed 7 years ago by bill

In [30109]:

Fix builder's handling of strings in staticHasFeatures, and add new "css-user-select" has() flag from [30098] to webkitMobile build, fixes #16421 and refs #15990 on 1.8 branch !strict

comment:16 Changed 7 years ago by Kenneth G. Franqueiro

RE [30108] and [30109]; while I was inclined to do the same thing, I am wondering if this was actually a design decision by Rawld.

I'm also a little nervous about applying that to 1.8 since it technically changes behavior (though I doubt too many people have experimented with pre-setting has features to strings yet anyway, and if they did they probably would've done it "wrong").

Rawld, do you have any thoughts on this?

comment:17 Changed 7 years ago by Kenneth G. Franqueiro

In [30162]:

Improve dom.setSelectable impl w/ feature detection and test page on 1.7 branch; refs #15990 !strict

comment:18 Changed 7 years ago by Kenneth G. Franqueiro

Milestone: 1.7.51.6.2

Came up with and committed a set of changes for 1.7.5 in [30162]. The 1.8 patch failed on 1.7 because 1.7 still uses the old webkitMobile pragma (the webkitMobile profile was added in 1.8).

I've replaced setSelectable with the new has test and implementation, with exclude/include pragmas placed such that builds run with webkitMobile=true will get a simplified block of code which just hard-codes the has test's value and sets the setSelectable function directly. Test-ran against source across browsers and against a build with webkitMobile=true in Chrome; both are working.

Marking this 1.6 now to address there next.

comment:19 Changed 7 years ago by yurychika

When will the fix be available in 1.8 version?

comment:20 Changed 7 years ago by bill

It's already available, in the 1.8.3 release.

comment:21 Changed 6 years ago by dylan

Milestone: 1.6.21.9

Is there any reason this is still open, other than to get Rawld's thoughts on it?

comment:22 Changed 6 years ago by bill

It's only open because in comment:18 @kgf left it open to backport it to 1.6.2. But that was three months ago, so we should probably close it as fixed in 1.7.5, and then if @kgf decides to backport the fix to 1.6.2 he can change the milestone.

comment:23 Changed 6 years ago by Kenneth G. Franqueiro

Keywords: ie9 ie10 added
Milestone: 1.91.7.5
Resolution: fixed
Status: assignedclosed

Yes, Bill is right, and this is already fixed in trunk. See, this is why I lament our inability to reference a ticket to multiple milestones - it causes confusion like this :) (and also makes it impossible to really see what got fixed in any given one).

If you'd prefer I close this rather than leave it open, I'll do so and mark it as 1.7.5. I was only trying to keep things in need of further backporting evident (since I thought we were going to try to backport to 1.4-1.6 within 3 months), though I guess we're going to have a rough time of that with the IE10 fixes anyway.

comment:24 Changed 6 years ago by Kenneth G. Franqueiro

Also FWIW the thing I wanted Rawld's input on really was #16421. It looks like the same concern was already voiced there by Colin and addressed by rolling back the change.

Note: See TracTickets for help on using tickets.