752988 - Focus lost when closing notification bars

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Jason Orendorff [:jorendorff]]

Steps to reproduce:
1. pref devtools.chrome.enabled = true
2. open Scratchpad
3. menu Environment -> Browser
4. click the X to dismiss the warning
5. Cmd+A or Ctrl+A to select the contents of the scratchpad
6. hit Delete/Backspace

Expected: the contents of the scratchpad should be deleted in step 6

Observed: everything works fine through step 5, but on hitting Delete in step 6, nothing happens

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

The close button in the toolkit widget notification doorhanger was stealing focus from the scratchpad editor.

"4. click the X to dismiss the warning"

 I added an event to be fired in when its close button is clicked, then listened for it in the scratchpad and set the focus back accordingly.

This tests out on my local WIN machine.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 646955 [details] [diff] [review]
Patch (v1)

The notification not giving back focus is a bug that should be fixed entirely within notification.xml.

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v2)

In that case, this works also. I built the original patch, as I'm also looking at Bug 609930 - Add AlertClosed event to notification boxes, and thought to make use of the new event.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

I imagine "tabbable" was added for a reason (keyboard accessibility?). Have you checked hg blame?

Comment 5

[Mark Capella [:capella]]

I wondered about a11y, having done a buncha bugs for them, and this did seem to imply effects in general outside the scratchpad might occur. The first patch fixes both (I believe) but there might be a better / best way I'm missing yet.

Comment 6

[Mark Capella [:capella]]

I guess I didn't answer your question. HG blame command line shows the line revise number is 1, but then I found on github blame Bug 347247 - No way to dismiss a notification without a mouse ... 

Must be for notificationboxs in general... in Scratchpad I tried and couldn't find a way to tab to the close field while decided if this was a good solution for this bug ...

Comment 7

[Rob Campbell [:rc] (:robcee)]

yeah, I can't review this in toolkit. One of the toolkit peers should take a look. Like gavin suggests, I would imagine "tabbable" was added for a11y reasons (haven't tested, can you tab out of the close widget on the notification box?). I don't feel comfortable making this change for the scratchpad without another reviewer.

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 647045 [details] [diff] [review]
Patch (v2)

Cancelling review, needs a toolkit peer.

Comment 9

[Mark Capella [:capella]]

Comment on attachment 647045 [details] [diff] [review]
Patch (v2)

Asking feedback request from the Toolkit/Widget peer name I've seen in IRC.

(And summarizing) So Patch (v2) won't be correct due to the history I found in comment 6 showing that there's an a11y reason to leave the tabbable parm in place on the close button in the notification box.

That leaves me the original Patch (v1) as a workable option with most changes on the ScratchPad side, while also completing an open Bug 609930 - Add AlertClosed event to notification boxes on the XP Toolkit/Widgets: XUL side for the moment.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 647045 [details] [diff] [review]
Patch (v2)

The button needs to be focusable for accessibility.

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch

I tested this with the scratchpad and in the tabbed browser, which suffers from the same problem.

Comment 12

[Mark Capella [:capella]]

Thank you! I was hoping you might agree to take it :)

Comment 13

[Neil Deakin]

Comment on attachment 647303 [details] [diff] [review]
patch

I assume this is caused by bug 570835?

Comment 14

[Dão Gottwald [:dao]]

(In reply to Neil Deakin from comment #13)
> Comment on attachment 647303 [details] [diff] [review]
> patch
> 
> I assume this is caused by bug 570835?

Yes, I'll add a comment with a reference to that bug.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/765da436c5b5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/765da436c5b5