Closed Bug 752988 Opened 8 years ago Closed 8 years ago

Focus lost when closing notification bars

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jorendorff, Assigned: dao)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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
Assignee: general → nobody
Component: JavaScript Engine → Developer Tools: Scratchpad
Product: Core → Firefox
QA Contact: general → developer.tools.scratchpad
Version: Other Branch → unspecified
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #646955 - Flags: review?(rcampbell)
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.
Attachment #646955 - Flags: review?(rcampbell) → review-
Attached patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #646955 - Attachment is obsolete: true
Attachment #647045 - Flags: review?(rcampbell)
I imagine "tabbable" was added for a reason (keyboard accessibility?). Have you checked hg blame?
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.
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 ...
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 on attachment 647045 [details] [diff] [review]
Patch (v2)

Cancelling review, needs a toolkit peer.
Attachment #647045 - Flags: review?(rcampbell)
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.
Attachment #647045 - Flags: feedback?(bzbarsky)
Comment on attachment 647045 [details] [diff] [review]
Patch (v2)

The button needs to be focusable for accessibility.
Attachment #647045 - Flags: feedback?(bzbarsky) → review-
Attached patch patchSplinter Review
I tested this with the scratchpad and in the tabbed browser, which suffers from the same problem.
Assignee: markcapella → dao
Attachment #647045 - Attachment is obsolete: true
Attachment #647303 - Flags: review?(enndeakin)
Component: Developer Tools: Scratchpad → XUL Widgets
Product: Firefox → Toolkit
Thank you! I was hoping you might agree to take it :)
Summary: Chrome Scratchpad focus bug → Focus lost when closing notification bars
Comment on attachment 647303 [details] [diff] [review]
patch

I assume this is caused by bug 570835?
Attachment #647303 - Flags: review?(enndeakin) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/765da436c5b5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.