Ed Lee :Mardak Reporter
	Description • 14 years ago

Should be able to type something and hit enter to finish a prompt as well as indicating that can be done with the solid blue ok button.

http://hg.mozilla.org/mozilla-central/rev/fc2988ab64e5#l3.188

	Ed Lee :Mardak Reporter
	Comment 1 • 14 years ago

Doesn't do too much good with bug 613767 though.

	Ed Lee :Mardak Reporter
	Comment 2 • 14 years ago

Seems to work:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/edward.lee@engineering.uiuc.edu-8e46160a4c77/tryserver-macosx64/firefox-4.0b8pre.en-US.mac64.dmg

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 14 years ago

Is this actually a regression? The old code for this looks the same as what we currently have:

http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/10fd22d17902/toolkit/content/commonDialog.js#l199

	Ed Lee :Mardak Reporter
	Comment 4 • 14 years ago

There's a default dialog button "accept":
http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/10fd22d17902/toolkit/content/widgets/dialog.xml#l52

Not sure where it's getting read out though.

	Justin Dolske [:Dolske] Assignee
	Comment 5 • 14 years ago

Stealing this from Mardak (yoink!) as I was looking at the general issue and just ended up fixing things.

I think I misunderstood what the |default| button attribute stuff was doing before, but I get it now.

This patch ensures tab-modal prompts always have a default button set and will use it when enter is pressed. Additionally, tab-modal prompts should listen for blur, so that if another part of the browser UI becomes focused (eg, the URL bar) we don't want to indicate that the default button is active. [And I made the focus listener work on OS X (previously ifdef'd out) so that un-blurring the prompt works).

Probably want a followup to make clicking anywhere on the prompt / blurred background focus the prompt, but the right thing seems to happen when tabbing or clicking the prompt() text input.

	Ed Lee :Mardak Reporter
	Comment 6 • 14 years ago

cc faaborg expected focus and default buttons behavior on os x

	Ed Lee :Mardak Reporter
	Comment 7 • 14 years ago

dolske said to comment here, but I might need to file separate bugs for focus. I've attached screenshots of the window-modal dialogs for alert, confirm, and prompt as well as tab-modal dialogs on the right.

I'm using the os x preference "Full Keyboard Access" that allows buttons to be focused.

window-modal:
alert: default = OK, focus = prevent additional checkbox
confirm: default = OK, focus = cancel [couldn't get the checkbox to appear]
prompt: default = OK, focus = textbox [no checkbox]

tab-modal:
alert: default = OK, focus = OK
confirm: default = OK, focus = OK
prompt: default = none?, focus = textbox

I would expect the default to always be set on OK. And the textbox for the prompt to be focused. Questionable for the checkbox on alert and confirm though..

	Justin Dolske [:Dolske] Assignee
	Comment 8 • 14 years ago

(In reply to comment #7)

> dolske said to comment here, but I might need to file separate bugs for focus.

Yeah, let's use bug 615026 for your focus issue. Don't think that needs to block this patch from landing, and they're going to be separate changes.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 14 years ago

I don't understand why you're making this code diverge so much from the dialog.xml code is was copied from. The first hunk makes sense (to address the first part of bug 59314 comment 159), but I don't understand why the other changes are needed. Is the idea to intentionally have different behavior for tab modal prompts than for existing dialogs on Mac?

	Justin Dolske [:Dolske] Assignee
	Comment 10 • 14 years ago

(In reply to comment #9)
> I don't understand why the other
> changes are needed. Is the idea to intentionally have different behavior for
> tab modal prompts than for existing dialogs on Mac?

The blur handler isn't needed in dialog.xml because you can't move focus outside of the prompt window anyway (well, you can blur the entire window, but that's handled elsewhere). The focus handler is similarly needed on OS X because now you
can shift focus back to something.

For example, with javascript:prompt("ok"), tabbing (in a tab-modal prompt) will eventually take you out of the prompt, and into the URL bar. And if you keep going, you'll get back to the prompt again.

Together these handlers clear the |default| attribute so the button isn't visibly active when the prompt contents don't have focus.

The CommonDialog.jsm change (which I don't think you're asking about) is ensuring there's always a visible default button, even when there's a text field. The "XXX" comment there was actually a bug. :( dialog.xml does this initially via |this.defaultButton = this.defaultButton|.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 11 • 14 years ago

Comment on attachment 493174 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/prompts/content/tabprompts.xml b/toolkit/components/prompts/content/tabprompts.xml

>-#ifndef XP_MACOSX
>         <handler event="focus" phase="capturing">

>+            // If we're focusing outselves, we're done.
>+            if (event.originalTarget == button)
>+                return;
>+
>+            // If focus shifted to something that isn't a button, style the
>+            // default button as the default again.
>+            //
>+            // Note that on OS X, buttons are not focusable, so this function
>+            // will always keep the default button styled when the tab prompt
>+            // is focused.
>+            let someButtonFocused = event.originalTarget instanceof Ci.nsIDOMXULButtonElement;
>+            button.setAttribute("default", !someButtonFocused);

This isn't quite right. On Mac with FKA, when the tabprompt vbox gets focus, we activate the default styling for the button, but then when you tab again and actually focus the default button, the vbox blur removes the styling and it isn't re-added (because we take the early return). This seems like a problem that would affect all platforms, and I confirmed that the "default" attribute isn't being set on Linux when you tab to the default button, but the focused and default styling are nearly the same, so I think it's just harder to notice there (or maybe the focused/default styling interact strangely?). Haven't tested Windows.

It seems to me like the goal is to set the "default" attribute on the default button when it will be triggered by Enter, which seems to map to "when focus is in the tabprompt, but not on another button" on Linux/Windows, and "when focus is in the tabprompt" on Mac (regardless of FKA, apparently... that's kind of weird and I don't understand it).

>diff --git a/toolkit/components/prompts/src/CommonDialog.jsm b/toolkit/components/prompts/src/CommonDialog.jsm

>-        // If there are no input fields on the dialog, select the default button.
>-        // Otherwise, select the appropriate input field.
>-        // XXX shouldn't we set an unfocused default even when a textbox is focused?
>+        // Set the default button

So this is a behavior change for the xulDialog case (obeying the specified defaultButtonNum when there are text fields, rather than relying on the default dialog.xml behavior of using the accept button, as commonDialog used to do). I guess that's a bug fix, though, since I can't think of any reason why we should ignore the specified default button in that case. Though actually, defaultButtonNum can only be specified via confirmEx's buttonflags, and confirmEx dialogs don't have text fields, so that case was never hit in practice.

>+        let b = 0;
>+        if (this.args.defaultButtonNum)
>+            b = this.args.defaultButtonNum;

You could use this.args.defaultButtonNum || 0 here too.

>+        // Set default focus / selection. Buttons don't get focus on OS X.

This comment is a bit misleading (buttons can get focus if the FKA system pref is flipped). As far as I can tell the justification for not focusing on Mac is just that it's unexpected that default buttons are focused automatically ("because it looks weird", bug 335089), so just "Don't focus the default button on Mac" or somesuch would be better.

	Justin Dolske [:Dolske] Assignee
	Comment 12 • 14 years ago

(In reply to comment #11)

> It seems to me like the goal is to set the "default" attribute on the default
> button when it will be triggered by Enter, which seems to map to "when focus is
> in the tabprompt, but not on another button" on Linux/Windows, and "when focus
> is in the tabprompt" on Mac (regardless of FKA, apparently... that's kind of
> weird and I don't understand it).

Blargh. I think the problem here is that XUL buttons ignore VK_RETURN on OS X (see nsButtonBoxFrame::HandleEvent), but trigger the button on other platforms. So even with FKA: if you focus the Cancel button, the Ok (default) button still needs to be tagged as default because enter will trigger it (whereas space will trigger cancel). On Windows/Linux both space and enter trigger the focused button.

I was trying to make the platforms act more alike here, but I think I'll just punt that for another bug. Maybe bug 613704.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 14 years ago

Comment on attachment 496079 [details] [diff] [review]
Patch v.3

We need some way to ensure that the prompt in general is focused on Mac, because otherwise the dialog can't be closed if the focus starts out elsewhere (e.g. focus a link in content, enter javascript:alert(1)._ in URL bar, Enter activates link rather than activating dismissing dialog). The button.focus() in commonDialog.jsm's !xulDialog case is what's currently making that work (by focusing the *button*, though, which is wrong).

I tried adding a this.focus() in tabprompts.xml init(), but that doesn't seem to work for some reason.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 15 • 14 years ago

(but with that problem solved, that patch looks good)

	Justin Dolske [:Dolske] Assignee
	Comment 16 • 14 years ago

I added some quick debugging to look at what gets focused on OS X window-modal dialogs, and it was conveniently the info.body <description>. So let's just explicitly focus that on OS X... Needs your patch from bug 613742 to actually make the focus() work, though.

	Justin Dolske [:Dolske] Assignee
	Comment 17 • 14 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/ce9e25e3cd10