312846 - Pressing Enter when a UI-link is focused inside a wizard advances the wizard

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Asaf Romano (gone)]

While working on bug 311288, I noticed the following behavior:

1. Either in the Update Found page or in the Ready To Install page, focus the
Details link.
2. Press Enter.

Results: 1) The link is activated (desired). 2) the wizard advances.

In the Ready to Install page, the "side-effect" is a restart of the application.

Comment 1

[Asaf Romano (gone)]

Assuming Enter is the only kbd-way to open ui links -> sec508.

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Comment 3

[Asaf Romano (gone)]

Attachment: diff -w

Comment 4

[Asaf Romano (gone)]

Checking in text.xml;
/cvsroot/mozilla/toolkit/content/widgets/text.xml,v  <--  text.xml
new revision: 1.21; previous revision: 1.20
done

Comment 5

[Scott MacGregor]

we'll need a trunk verification on this in tomorrow's bits before we'd consider
this patch for the branch.

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 199936 [details] [diff] [review]
diff -w

We're inclined against this patch since the impact limited to a small group of
users and the fix is changing something that could have widespread impact.

Comment 7

[Asaf Romano (gone)]

Asa, what's the reasoning? there are very few ui-links in Fx/Tb and this only
touches the case where the the Enter key is pressed on those. The current
behavior does have a pretty bad widespread impact though - ui links (a new
widget in 1.8) aren't functional in dialogs and wizards.

FYI, another place where it's broken is EM's about dialog.

Comment 8

[Scott MacGregor]

we were pretty scared to take changes to the text widget including how events
are getting handled in the widget this late in the release cycle without any
time to see if things work properly. We felt the impact of the bug didn't
justify regressions to everyone else who uses text widgets. 

Comment 9

[Asaf Romano (gone)]

Scott, the current "text-link" binding is pretty new (added by aaronlev in 1.5a2
iirc) and for that reason isn't used a lot yet (there are users of
type="text-link" onclick="foo" but this change doesn't affect them at all).

I've tested this in SWU and EM/TM.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 199936 [details] [diff] [review]
diff -w

Renominating for tomorrow's triage assuming this info matters. I will try to
test the few text-links in thunderbird till then.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 199936 [details] [diff] [review]
diff -w

we'll try to get good test coverage on this when doing our update testing. Are
there any places outside of the update wizard that use this?

Comment 12

[Asaf Romano (gone)]

Attachment: make things a bit safer

Firing open directly might be the very wrong thing to do here in some cases, so
i'm reverting that part.

Comment 13

[Asaf Romano (gone)]

(In reply to comment #11)
> (From update of attachment 199936 [details] [diff] [review] [edit])
> we'll try to get good test coverage on this when doing our update testing. Are
> there any places outside of the update wizard that use this?
> 

In the Extension Manger and in Thunderbird's Preferences window.

Comment 14

[Asaf Romano (gone)]

Fixed on branch. I will get a post-checkin review on the follow-up patch and
will land it on the trunk then.

Comment 15

[Asaf Romano (gone)]

Fixed on trunk too.

Comment 16

[Scott MacGregor]

Asaf, did you mean to check in the dump statement for textbox.xml as part of this change?

Comment 17

[Asaf Romano (gone)]

Your checkin, actually ;)
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/toolkit/content/widgets&command=DIFF_FRAMESET&file=text.xml&rev2=1.20&rev1=1.19\
 (v1.20)

Comment 18

[Scott MacGregor]

sorry bad cvs blame data. I thought that dump looked familiar! I'll take it out. 

Comment 19

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381

Comment 20

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381