Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9654 closed defect (fixed)

[PATCH][CCLA]dijit/_base/focus.js: Fails in getBookmark, getFocus, on Opera

Reported by: Jared Jurkiewicz Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.4
Component: Dijit Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by Jared Jurkiewicz)

dijit/_base/focus.js: Fails in getBookmark, getFocus, on Opera

This error came out of investigations on fixing Editor bugs. One thing I noticed was when I was prototyping out a replacement 'Indent' handler plugin was that I needed to make use of the customUndo logic since the manipulation required working with DOM in the iFrame, which wasn't undoable by default in the browser.

What I discovered was the customUndo logic in Editor fails on Opera. specifically it fails in:

Editor.getBookmark: The line (378): var b=dojo.withGlobal(this.window,dijit.getBookmark);

Throws error:

[ Error: Statement on line 62: Type mismatch (usually non-object value supplied where object required) Backtrace: Line 62 of eval script bookmark = range.getBookmark(); ... Line 87 of eval script return callback.apply(thisObject, cbArguments
[]); ... Line 58 of eval script return dojo.withDoc.call(null, globalObject.document, callback, thisObject, cbArguments); Line 379 of eval script var b=dojo.withGlobal(this.window,dijit.getBookmark); Line 407 of eval script this._steps.push({'text':this.savedContent,'bookmark':this._getBookmark()}); Line 256 of eval script this._beginEditing(); Line 238 of eval script this.editor.execCommand(this.editor._normalizeCommand(this.command), choice); ... Line 133 of eval script function(){ return method.apply(scope, arguments []); } ... Line 45 of eval script lls[i].apply(this, arguments); Line 312 of eval script this.onChange(newValue); ... Line 133 of eval script function(){ return method.apply(scope, arguments []); } ... ]

This fails because the getBookmark code in _base/focus.js mistakenly detects Opera as IE (By assuming that document.selection only exists on IE. This is wrong, it also exists on Opera.) The problem is that code path only works on IE, so Opera blows up. This is the same discovery I made (and fixed), in tracker:

http://bugs.dojotoolkit.org/ticket/9520

Regarding selection (Very similar code in there).

So ... the fix is also the same, use dojo.isIE instead to select the IE path. I know this goes against the push by some to use 'feature detection' but I'm not sure how else to detect it's IE only and not mistakenly use IE codepaths for Opera.

When I applied a fix to focus, I was able to use customUndo with Editor. It may fix other Opera bugs where code in dojo makes use of dijit.getBookmark and the like. Patch forthcoming.

Attachments (6)

focus.patch (1.1 KB) - added by Jared Jurkiewicz 10 years ago.
Minor patch to focus to fix Opera issues.
focus2.patch (4.2 KB) - added by Jared Jurkiewicz 10 years ago.
A patch that doesn't use dojo.isIE, but instead just checks for range API first.
focus3.patch (5.5 KB) - added by Jared Jurkiewicz 10 years ago.
Enhanced patch, fixes restoring selections in input elements on Moz and Opera.
focus4.patch (5.5 KB) - added by Jared Jurkiewicz 10 years ago.
Tweak on patch 3 for a focus restore issue seen on IE when restoring caret position in editor (Showed up with edtior plugins that had drodowns. Eg: LinkDialog?).
focus5.patch (9.9 KB) - added by Jared Jurkiewicz 10 years ago.
Refactored a bit based on Bill's wants. Updated the unit tests to verify input elements are properly restored, added UT to verify that text selection in the page is also restored.
focus6.patch (9.8 KB) - added by Jared Jurkiewicz 10 years ago.
Same as focus5.patch with small reduction

Download all attachments as: .zip

Change History (34)

comment:1 Changed 10 years ago by Jared Jurkiewicz

Description: modified (diff)

Changed 10 years ago by Jared Jurkiewicz

Attachment: focus.patch added

Minor patch to focus to fix Opera issues.

comment:2 Changed 10 years ago by bill

Great, thanks for getting opera to work.

I actually tried that change myself but it still didn't seem like Opera was saving the selection. I guess I must have messed something up. I'm assuming that your fix fixes the unit test?

FYI, ISTM that FF doesn't save the selection but it still works because a textarea/input doesn't lose selection just because it loses focus. And that's the same thing I saw with Opera.

comment:3 Changed 10 years ago by Jared Jurkiewicz

Which UT were you running? I'll explore it more. What I know this fixes is in Editor, is the customUndo. If you set that to say: dojo.isIE
dojo.isOpera, it'll now work. Before it failed because the getBookmark would throw an exception.

comment:4 Changed 10 years ago by Jared Jurkiewicz

Ah, I believe you mean: tests/_base/test_FocusManager.html

And I see what is going on. The reason we never saw a problem before with that UT is that isCollapsed, the IE block, was throwing an error. But, there's that catch that catches it and returns true. So it always flags the area as collapsed (no text), and therefore never called getBookmark in this case.

I logged the error and got:

[ Error: Statement on line 35: Cannot convert undefined or null to Object Backtrace: Line 35 of eval script return !(s.type == 'Text' ? range.htmlText.length : range.length); Boolean ... Line 87 of eval script return callback.apply(thisObject, cbArguments
[]); ... Line 58 of eval script return dojo.withDoc.call(null, globalObject.document, callback, thisObject, cbArguments); Line 133 of eval script return { Line 9 of inline#1 script in file://localhost/X:/dojo/trunk/dijit/tests/_base/test_FocusManager.html?: In function save savedFocus = dijit.getFocus(fakeWidget); ... ]

So ... going back to my update to it to see what we get for collapsed instead...

comment:5 Changed 10 years ago by Jared Jurkiewicz

Okay, I know what is happening!

for the Range API on both FF and Opera (and this may be spec), selecting IN an input element always returns a collapsed selection. Meaning if you select the text in the input element You can't actually save that selection state. This is consistent with what you saw.

This also showed me how I can demonstrate how the fix works...

Don't select in the inputs!

Okay, so here's how you can see the fix working:

1.) Fresh dojo tree (no patch). 2.) Load in FireFox?, push the delayed selection save button (2) 3.) Quickly select the text in the page. 4.) Wait for selection save. 5.) Select off that to clear the selection and focus. 6.) Press the restore selection/state.

In FireFox? and IE, you'll see the selection of the text return. In Opera, it won't restore the selection (because it errored in the IE block and returned collapsed)

Now, apply my patch and do the same test again. Now Opera will work too. :-)

I'm going to look at if there's a way to save selection IN an input element too, but I'm not promising anything there.

I also have a new patch that redid the functions based off of Liu's suggestion of test for Range API first. This gets rid (thankfully), of needing dojo.isIE.

Select some of the text above the input boxes, part of the description paragraph.

Changed 10 years ago by Jared Jurkiewicz

Attachment: focus2.patch added

A patch that doesn't use dojo.isIE, but instead just checks for range API first.

comment:6 Changed 10 years ago by liucougar

wrt the latest patch: how about move the "feature detection" code outside of each function:

instead of doing check for dojo.global.getSelection/dojo.doc.selection in each function, how about doing that once in the module, something like:

dojo.feature.W3CRange=Boolean(dojo.global.getSelection)
dojo.feature.IERange=Boolean(dojo.doc.selection)

then use dojo.feature.W3CRange/IERange in each function?

comment:7 Changed 10 years ago by Jared Jurkiewicz

Well, shouldn't that go further up into dojo instead of just focus.js? Once it's present, we can use it in the selection code of Editor too.

comment:8 Changed 10 years ago by bill

attachment focus2.patch added
A patch that doesn't use dojo.isIE, but instead just checks for range API first.

Sounds good to me.

You could add a dojo.feature.IERange flag if you want but that just sounds like a more verbose way to write dojo.isIE.

comment:9 Changed 10 years ago by Jared Jurkiewicz

Okay! So I worked a bit more on it ... and I fixed the FireFox? and Opera issue with restoring selections inside textarea and input fields. The problem is both of those browsers don't return selected text from those (Collapsed is true, no range). Instead, you have to dig a tiny bit deeper.

So basically I look at if it's Moz or Opera, then I look at the node selected (dijit._curFocus). If it turns out to be a textarea or input, I look at the start/send selection rages of that element. If end > start, it has some selection and I return collapsed is actually false.

Then in getBookmark I build a pseudoRange object that contains the node, its start and end selection indexes, and a boolean marker for easy detect. That's all stored in the getFocus() object.

I then updated dijit.moveToBookmark() to detect that psuedoRange, and if present, resets the start/end selection on the node to what was saved off. End result: Selections are now restored in textarea and input fields on Mozilla and Opera!

So ... with that, I think we have the getFocus storing now working completely consistently across browsers.

Patch forthcoming!

Changed 10 years ago by Jared Jurkiewicz

Attachment: focus3.patch added

Enhanced patch, fixes restoring selections in input elements on Moz and Opera.

comment:10 Changed 10 years ago by Jared Jurkiewicz

Summary: dijit/_base/focus.js: Fails in getBookmark, getFocus, on Opera[PATCH][CCLA]dijit/_base/focus.js: Fails in getBookmark, getFocus, on Opera

comment:11 Changed 10 years ago by Jared Jurkiewicz

I may have a followon patch to this, as I found that the selection restore doesn't *quite* work right in IE (Discovered when using editor plugins with dropdowns). Testing a fix to that now.

Changed 10 years ago by Jared Jurkiewicz

Attachment: focus4.patch added

Tweak on patch 3 for a focus restore issue seen on IE when restoring caret position in editor (Showed up with edtior plugins that had drodowns. Eg: LinkDialog?).

comment:12 Changed 10 years ago by bill

Thanks for all the work on this. The code looks good but I just wish the code size could be smaller.

Maybe combining getBookmark() / isCollapsed() into one method? (Then define isCollapsed() as !!getBookmark(), assuming that getBookmark() returns null for empty selections). Plus standard tricks like using tertiaries, etc. Also, can the opera/moz branch run for all W3C compatible browsers or does it only work on opera and moz?

comment:13 in reply to:  12 Changed 10 years ago by liucougar

Replying to bill:

Maybe combining getBookmark() / isCollapsed() into one method? (Then define isCollapsed() as !!getBookmark(), assuming that getBookmark() returns null for empty selections).

getBookmark() should work for cases where the range is collapsed: sometimes we need to restore cursor position even if it was collapsed

comment:14 Changed 10 years ago by bill

Fair enough, I still think the code can be reduced though. Perhaps getBookmark() could return an object w/a isCollapsed flag in it, or something like that.

comment:15 Changed 10 years ago by Jared Jurkiewicz

getBookmark doesn't always return for collapsed ranges, actually. At least in respect to selections in inputs. There is no range (selection.isCollapsed is true in that case, by the by). But, selection.getRangeAt(0) faults, I tried that to see what it returned with. So we have to keep the checks for opera/moz and its associated introspection of inputs in that corner case (bleh).

I'd like it to be smaller too, of course, and I've been pondering how to reduce it. Oh well, at least it works more correctly now and we can continue on reductions as we go.

comment:16 Changed 10 years ago by Jared Jurkiewicz

And to answer your question, as far as I know, that moz/opera branching really is moz/opera specific. But I'll check tomorrow and see if it also works in general for safari as well for that corner case.

comment:17 Changed 10 years ago by Jared Jurkiewicz

Okay, was able to do some byte reductions on it by collapsing out the mos/opera check, as that path only goes down if it's collapsed anyway (and Safari isn't collapsed when selecting in textareas, etc), so it works.

Also shortened several varnames and the like. Still very understandable in the functions (since they're short), but cuts off quite a few bytes.

So, I reduced off about 1K from the focus4.patch. Running tests not to validate it, and if it all looks sane in IE, FF, Opera, WebKit?, I'll be attaching a focus5.patch.

If you're then satisfied enough with it, I'll commit the changes. It's still a bit larger than the original I started from, but it now behaves better. :)

comment:18 Changed 10 years ago by Jared Jurkiewicz

Er, running tests NOW to validate, sorry.

Changed 10 years ago by Jared Jurkiewicz

Attachment: focus5.patch added

Refactored a bit based on Bill's wants. Updated the unit tests to verify input elements are properly restored, added UT to verify that text selection in the page is also restored.

comment:19 Changed 10 years ago by Jared Jurkiewicz

Latest patch reduces down the code a bit (still larger than the original, but not by much), adds in updated UT to validate that selections, even in inputs, is properly restored. Etc.

Changed 10 years ago by Jared Jurkiewicz

Attachment: focus6.patch added

Same as focus5.patch with small reduction

comment:20 Changed 10 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [19678]) Cleanup/updates to dijit._base.focus. Fixed longstanding FF bug with restoring selections in input/textareas, fixed opera problem. fixes #9654

comment:21 Changed 10 years ago by Nathan Toone

(In [19752]) Refs #9654 - can not get selectionStart of non-text input elements (ie radio, button, etc...apparently not password or file, either)

comment:22 Changed 10 years ago by bill

Milestone: tbd1.4

comment:23 Changed 10 years ago by bill

Resolution: fixed
Status: closedreopened

Starting from [19678] there are problems on IE, specifically that Dialog doesn't restore focus to buttons when it's closed. Actually, it does restore focus for an instant, but then it tries to restore the bookmark and that makes everything lose focus.

There's also a differ issue that started *after* [19678] where if the Dialog appears while focus is on an <input>, then when the dialog is closed (on IE8, maybe other IE's too), all the text on the screen is selected. This can be tested on test_Dialog.html by using the "Programmatic Dialog (3 second delay)" button.

comment:24 Changed 10 years ago by bill

Owner: set to Jared Jurkiewicz
Priority: normalhigh
Status: reopenednew

comment:25 Changed 10 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [20335]) Fixing issue with IE, Dialogs, and input/button/textarea elements. fixes #9654

comment:26 Changed 10 years ago by Jared Jurkiewicz

(In [20352]) More tweaks to handling slection on IE. fixes #9654

comment:27 Changed 10 years ago by Jared Jurkiewicz

(In [20438]) Editor was somewhat broken by the _base/focus.js refactor, it actually pries into the bookmark format. Most notable is that it broke customUndo on W3C compliant browsers. refs #9654

comment:28 Changed 10 years ago by Jared Jurkiewicz

(In [20439]) Fixing jslint complaints, related to work done to fix focus behavior. refs #9654

Note: See TracTickets for help on using tickets.