263060 - bookmarklet select stopped working (editor not initialized correctly)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Peter van der Woude [:Peter6]]

bookmarklet:

javascript:(function() { var title = document.title; function
htmlEscape(s){return s;}; var
OS=document.getElementById(%22op_sys%22).value.substring(0,3) ;
document.write(%22<form name=f><textarea name=ta cols=120
rows=5></textarea></form>%22); document.close(); document.f.ta.value =
%22[*][url=%22 + location.href + %22]BRANCH:#%22 + /\d+/(title)[0] + %22
[/url]%22 + htmlEscape(/-.*/(title)[0].slice(2)) +%22 [%22+OS +%22]%22+ %22%22;
document.f.ta.select(); })()

(original can be found on Jesse's website)

repro:
1. make this bookmarklet 
2. open a random bug
3. press on the bookmarklet
4. it will create a textfile I use for forums
5. notice, that despite "document.f.ta.select();" , the text is not selected

regression happened between 20041004 14PDT and 20041005 07PDT (Sweetlou builds)

Prime suspect bug 241625 "Open in tabs" doesn't focus content area in first tab 

CC [e-mail] and [e-mail]

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It's not bug 241625; I just tested both with and without it (it's a one-line
bookmarks.js change), and the bookmarklet worked fine in both cases.

Comment 2

[Peter van der Woude [:Peter6]]

I just rechecked
the sweetlou build downloaded 20041004 21:37 utc (14:37 pdt) shows the text
SELECTED.
the sweetlou build downloaded 20041005 14:02 utc (07:02 pdt) doesn't show the
text selected.
checked in (roughly in that period)
bug 228557 This one maybe 
bug 256218
bug 241625
bug 237204
bug 259039
bug 260591
bug 258787
bug 262750
bug 260704
bug 255572

Could bug 228557 have caused this Vlad ?

Comment 3

[Peter van der Woude [:Peter6]]

description in tinderbox for this was :
bug 228557 asynchronously initialize editor to avoid re-entering frame
construction.

checkin shaver 2004-10-04 14:10 PDT patches in :
mozilla/ layout/ html/ forms/ src/ nsTextControlFrame.h
mozilla/ layout/ html/ forms/ src/ nsTextControlFrame.cpp

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Could bug 228557 have caused this Vlad ?

It almost certainly did.  We need to force editor init on select() and other
operations that need an editor.  Those shouldn't be happening during frame
construction anyway.

Comment 5

[Mike Shaver (:shaver emeritus)]

I guess I should fix this, then, but I'll need to dig a bit and make sure I
understand why I broke this case.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Shaver, calling select() calls through to nsHTMLInputElement::SelectAll, which
calls nsTextControlFrame::SetProperty for the nsHTMLAtoms::select property. 
This calls SelectAllContents(), which bails because mEditor is null.

So we either need to force editor creation there or set a flag to re-call
SelectAllContents() once we have an editor...

Comment 7

[Boris Zbarsky [:bzbarsky]]

In general, there are various selection-related functions in nsTextControlFrame
that have similar issues (eg SetSelectionRange, SetSelectionStart,
SetSelectionEnd, GetSelectionRange, etc, etc).

Comment 8

[Mike Shaver (:shaver emeritus)]

OK, I think I'll just ReallyInitEditor there, since it can't happen during frame
construction (can it? can some XBL constructor do something unpleasant to access
the selection?).

Comment 9

[Boris Zbarsky [:bzbarsky]]

An XBL constructor can call select()....

Comment 10

[Mike Shaver (:shaver emeritus)]

So does that mean that they _can_ happen during frame construction?  Or does
comment 4 still hold.  Sorry, just want to be completely sure here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

To be frank, I'm not sure.  Constructor processing happens late, but not quite
_after_ frame construction.  So I don't know what will happen if it forces
editor init...

Comment 12

[Boris Zbarsky [:bzbarsky]]

But in general, constructor processing can do all sorts of weird shit; we have
bugs on it already (eg remove nodes that have had frames already created but not
yet inserted....)

Comment 13

[Mike Shaver (:shaver emeritus)]

OK, so I'm going to ReallyInitEditor here and we can fix this when we fix the
other stuff.  Patch coming up.

Comment 14

[Mike Shaver (:shaver emeritus)]

Of course, if mEditor is null we will also bail out of ReallyInitEditor.  I
think I need to spend some time in a debugger learning how this path is supposed
to work here.  Patch still coming, just not as _right_ up.

Comment 15

[Mike Shaver (:shaver emeritus)]

So looking at this code, not yet in the debugger, I'm wondering if the problem
is that we get through frame construction and call PostCreateFrames, which
_used_to_ synchronously initialize the editor, but now returns before editor
initialization is complete.  This means that when we get into select() we may
not have completed frame construction.

I'm not sure what the best way to fix this is.  We could just spin the event
loop in SelectAllContents() until we're initialized, I guess, but that feels
pretty gross.  To be perfectly honest, this whole thing feels pretty gross and
non-deterministic and fragile, but I still think it might be better than
crashing due to flashblock.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Of course, if mEditor is null we will also bail out of ReallyInitEditor.

mEditor is set in CreateAnonymousContent, no?  That happens before XBL
constructors and before any JS could possibly execute.

Comment 17

[Mike Shaver (:shaver emeritus)]

If that's the case, then how does what you describe in comment 6
(SelectAllContents finding a null mEditor) occur?  It seems like this access is
happening after before frame construction, but I guess I need to get into the
debugger after all.

Comment 18

[Mike Shaver (:shaver emeritus)]

Consensus: mEditor is not null, it just has no contents because it hasn't been
initialized.  On it, as the kids say.

Comment 19

[Mike Shaver (:shaver emeritus)]

Attachment: Force editor initialization for selection-access methods.

(That trick never works.)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161508 [details] [diff] [review]
Force editor initialization for selection-access methods.

r+sr=bzbarsky.

Comment 21

[Mike Shaver (:shaver emeritus)]

Comment on attachment 161508 [details] [diff] [review]
Force editor initialization for selection-access methods.

Requesting aviary approval for regression fix.

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 161508 [details] [diff] [review]
Force editor initialization for selection-access methods.

a=[e-mail] for aviary1.0, and where's the 1.7 approval? Ah, wrong
product....

/be

Comment 23

[Brendan Eich [:brendan]]

Tidying.

/be

Comment 24

[Mike Shaver (:shaver emeritus)]

Fixed on branch, closing.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

per bug 228557, this got checked in to 1.7 too (2004-10-08 15:01). adding
fixed1.7.x. also fixed on aviary branch 2004-10-08 14:52

Comment 26

[Johnny Stenback (:jst)]

This was backed out from the aviary branch, see bug 228557 for details.

Comment 27

[OstGote!]

This was backed out from the 1.7 branch too, see bug 228557 comment 55 for
details. So it is not fixed on 1.7 branch anymore, right? Reopen?

Comment 28

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20050218
Firefox/1.0 (FF1.0.1)

This WFM in FF 1.0.1

(sweetlou-aviary1.0.1 branch)

Comment 29

[Boris Zbarsky [:bzbarsky]]

[e-mail], you seem to be laboring under a fundamental misunderstanding of
the situation.  This bug was a regression caused by the fix for bug 228557. 
Both this fix and the fix in bug 228557 were backed out.  So this bug still
doesn't exist (since the changes causing it were backed out).