Closed
Bug 323805
Opened 19 years ago
Closed 17 years ago
Fix for bug 175893 breaks tab switching if content is not HTML
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 3 alpha7
People
(Reporter: bzbarsky, Assigned: enndeakin)
References
()
Details
(Keywords: fixed1.8.1, verified1.8.0.2, Whiteboard: [rft-dl])
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
neil
:
review+
bryner
:
superreview+
neil
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
The fix for bug 175893 introduced the following line into updateCurrentBrowser():
this.mCurrentBrowser.focusedElement.blur();
This throws if this.mCurrentBrowser.focusedElement is not an HTML element, which breaks tab-switching pretty badly. In particular, this breaks tab switching if an SVG anchor has focus, which is just not cool.
The solution is probably to put a try/catch around this line, but perhaps we should look for a not-only-HTML way to do this too.
To test, load URL in the URL bar, right click the anchor, dismiss the context menu while keeping the anchor focuse, and try switching tabs.
I'll file a separate bug for seamonkey, but make it depend on this one.
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8.0.2?
Flags: blocking-firefox2?
Comment 1•19 years ago
|
||
This seems to fix it.
Updated•19 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Updated•19 years ago
|
Attachment #208776 -
Flags: review?(aaronleventhal)
Comment 3•19 years ago
|
||
Comment on attachment 208776 [details] [diff] [review]
patch
I tested a SeaMonkey version of this and found that if an element in one tab had focus, then if I switched to another tab I was unable to use keyboard navigation until I explicitly clicked to give the new tab (or some content in the new tab) focus.
Updated•19 years ago
|
Attachment #208776 -
Flags: review?(aaronleventhal)
Comment 4•19 years ago
|
||
Ah sorry, I thought that DOMFocusIn and DOMFocusOut were working in Mozilla, but that's not the case yet, see bug 60212, comment 12:
"This patch implements DOMActivate for HTML input, button, and anchor elements,
and adds the infrastructure to support DOMFocusIn and DOMFocusOut (those events
are not dispatched currently, but that's next)."
That means that I don't see any other way to fix this then to put try..catch around the code.
Comment 5•19 years ago
|
||
Comment 6•19 years ago
|
||
Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection
FYI, I didn't include focus() protection in bug 323806 because the code that calls setFocus() checks to see if the element is null and it should already be null because of:
>+ this.mCurrentBrowser.focusedElement = null;
Comment 7•19 years ago
|
||
Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection
looks ok... it might be cleaner if we could just do
if "blur" in this.mCurrentBrowser.focusedElement
but I'm not sure what the security implications of that might be.
Attachment #209727 -
Flags: superreview+
Comment 8•19 years ago
|
||
Ah, thanks Andrew. That bit of the patch can be dropped on checkin then.
Updated•19 years ago
|
Attachment #209727 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•19 years ago
|
Attachment #209727 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Comment 9•19 years ago
|
||
Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection
Requesting approval for 1.8.0.2 and 1.8.1. This should be low risk. It's the same patch as was landed for SeaMonkey.
Attachment #209727 -
Flags: approval1.8.1?
Attachment #209727 -
Flags: approval1.8.0.2?
Updated•19 years ago
|
Attachment #209727 -
Flags: approval1.8.0.2? → branch-1.8.1?(bzbarsky)
Updated•19 years ago
|
Attachment #209727 -
Flags: approval1.8.1?
Updated•19 years ago
|
Attachment #209727 -
Flags: approval1.8.0.2?
Reporter | ||
Comment 10•19 years ago
|
||
Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection
I'm not a module owner or peer for this code, so I can't grant branch-1.8.1. Please ask someone who can?
Attachment #209727 -
Flags: branch-1.8.1?(bzbarsky)
Comment 11•19 years ago
|
||
Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection
Sure. Requesting branch-1.8.1 from Neil instead.
Attachment #209727 -
Flags: branch-1.8.1?(neil)
Comment 12•19 years ago
|
||
Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection
a=dveditz, platform parity, suite version already part of 1.8.0.1
Attachment #209727 -
Flags: approval1.8.0.2? → approval1.8.0.2+
Updated•19 years ago
|
Attachment #209727 -
Flags: branch-1.8.1?(neil) → branch-1.8.1+
Comment 13•19 years ago
|
||
*** Bug 324822 has been marked as a duplicate of this bug. ***
Comment 14•19 years ago
|
||
Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection
Can someone check this in on both branches please? I'll be away until the 22 Feb.
Comment 15•19 years ago
|
||
Reassigning to Boris for checkin on the logic that since he nominated this for 1.8.0.2 he must care about this bug.
Assignee: nobody → bzbarsky
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Reporter | ||
Comment 16•19 years ago
|
||
Fixed on branches. Leaving for trunk fix.
Assignee: bzbarsky → nobody
Keywords: fixed1.8.0.2,
fixed1.8.1
Reporter | ||
Comment 17•19 years ago
|
||
Er... apparently whoever posted that patch didn't so much test it. This caused bug 327139.
Updated•19 years ago
|
Whiteboard: [rft-dl]
Comment 18•19 years ago
|
||
Sorry, that was my rushed, pre-vacation testing. :-(
Comment 19•19 years ago
|
||
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060306 Firefox/1.5.0.2.
Looks like this patch also fixes bug 316191. With the same build above, I was able to verify that tab focus works as expected with testcases there as well (See https://bugzilla.mozilla.org/show_bug.cgi?id=316191#c15).
We still need to patch the Trunk.
Keywords: fixed1.8.0.2 → verified1.8.0.2
Comment 20•19 years ago
|
||
Is there a reason that this patch didn't land on the trunk? It should be OK to land as is, with the fix from bug 327139, right?
Comment 21•19 years ago
|
||
(In reply to comment #20)
> Is there a reason that this patch didn't land on the trunk?
"perhaps we should look for a not-only-HTML way to do this too."
Updated•18 years ago
|
Flags: blocking1.9?
Comment 22•18 years ago
|
||
*** Bug 316191 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Flags: blocking1.9? → blocking1.9+
Comment 23•18 years ago
|
||
need this fixed on trunk, carrying over blocking flag...
Flags: blocking-firefox3+
Updated•18 years ago
|
Assignee: nobody → enndeakin
Target Milestone: --- → Firefox 3 beta1
Comment 25•18 years ago
|
||
> this.mCurrentBrowser.focusedElement.blur();
Do we actually still need that line? It seems like a hack in order to avoid focus outline ghosting or something.
It's also causing bug 382007.
Comment 26•18 years ago
|
||
(In reply to comment #25)
>It seems like a hack in order to avoid focus outline ghosting or something.
Yes, the problem being that content.focus() still doesn't properly blur the previous element so we have to do it manually.
Comment 27•18 years ago
|
||
Is there a bug open for that?
Assignee | ||
Comment 28•18 years ago
|
||
Since for some reason this is assigned to me, what exactly still needs to be done here?
Comment 29•18 years ago
|
||
Neil,
Bug 316191 changed status and is marked as a dupe of this bug.
whilst the original bug 316191 is 'fixed' the fix may not be ideal:
Should the focus state of tab be retained on blur?
open https://bugzilla.mozilla.org/attachment.cgi?id=202809
navigate with keyboard and notice red ring
open an extra tab, change URI
select original tab
-- Notice focus has been lost...
would expect focus to be retained?
in this somewhat trivial example this may seem insignificant, however one may have navigated a fairly complex map, then checked a detail and be returning....
cheers
Comment 30•18 years ago
|
||
#29 in the general HTML case focus state is retained for each tab
Comment 31•18 years ago
|
||
#29 to compare:
open a new window, return to original, focus is retained,
so this - is - the expected behaviour.
Opera retains focus for each tab.
Clearly the current fix is broken in respect of this issue
Comment 32•18 years ago
|
||
(In reply to comment #28)
>Since for some reason this is assigned to me, what exactly still needs to be
>done here?
As I recall, you have a number of options ;-)
1. Make it so focusing the new tab (via content.focus() or some such) properly removes the focus from the currently focused element in the current tab.
2. Make it possible to remove the focus from the currently focused element without requiring focus or blur methods which only HTML or XUL elements have.
3. Use a hack similar to the compose window (which I believe advances focus into the frame element rather than focusing the content window).
Comment 33•18 years ago
|
||
Re: #32
Neil,
1. the current behaviour seems to mimic this suggestion, and as per #29-31 this appears wrong.
2. we would like to retain focus see (1), not remove, so this seems wrong.
3. no comment as it's not explicit, to me at least...
notwithstanding this there seems to be some confusion regarding use of terms such as blur and focus:
Navigating the content, or the application?
Comment 34•18 years ago
|
||
(In reply to comment #33)
>notwithstanding this there seems to be some confusion regarding use of terms
>such as blur and focus:
OK, so here's a quick summary. Only one element (or window, if no element in the window has focus) can have focus at a time. Each window remembers which element last had focus so that when you switch window it automatically restores focus to that element. However the tabbed browser can't emulate this, because only some elements have a scriptable method to restore focus.
What I actually touched on in comment #32 was that tabbed browser can't handle removing focus from the previous tab either, because in certain cases restoring focus doesn't remove the previous focus correctly.
Comment 35•18 years ago
|
||
#34
Neil,
"However the tabbed browser can't emulate this"
If the can't is absolute then this is a 'major' design fault. And needs to be marked and addressed as such.
tab emulates new window.
if tab cannot emulate new window then it isn't worth implementing.
because the general user will have to learn a new routine.
for instance I also tested by chance with http://www.peepo.co.uk where using CSS visually it appears that focus is retained, but it isn't. This will easily lead to a mess, unless the application behaves as expected.
to repeat:
as per #30 html already maintains focus within tabs
as per #31 opera maintains focus in svg and html
thanks for your explanation of comment #32 are there additional "in certain cases" bugs that depend on this one?
Reporter | ||
Comment 36•18 years ago
|
||
Perhaps window utils should have a method for focusing any element or something?
Comment 37•18 years ago
|
||
(In reply to comment #35)
>"However the tabbed browser can't emulate this"
>
>If the can't is absolute then this is a 'major' design fault. And needs to be
>marked and addressed as such.
Well, that was bug 175893 but we've since discovered it isn't foolproof.
(In reply to comment #36)
>Perhaps window utils should have a method for focusing any element or something?
Sounds like a plan ;-)
Assignee | ||
Comment 38•17 years ago
|
||
Add a windowutils focus method, and call it as a backup if focus/blur methods are not available.
Attachment #271832 -
Flags: review?(neil)
Comment 39•17 years ago
|
||
Comment on attachment 271832 [details] [diff] [review]
like so
>+ void focus(in nsIDOMNode aNode);
Are all nodes focusable, or only elements?
>+NS_IMETHODIMP
>+nsDOMWindowUtils::Focus(nsIDOMNode* aNode)
>+{
>+ if (mWindow) {
>+ nsIDocShell *docShell = mWindow->GetDocShell();
>+ if (docShell) {
>+ nsCOMPtr<nsIPresShell> presShell;
>+ docShell->GetPresShell(getter_AddRefs(presShell));
>+ if (presShell) {
>+ nsPresContext *pc = presShell->GetPresContext();
>+ if (pc) {
>+ nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
>+ pc->EventStateManager()->ChangeFocusWith(content,
>+ nsIEventStateManager::eEventFocusedByApplication);
>+ }
>+ }
>+ }
>+ }
>+}
No return value. (On my compiler this is a compile error...)
>+ if ("blur" in elem) {
>+ elem.blur();
>+ }
I guess you're doing this as a quick shortcut?
>+ else {
>+ var content = window.content;
>+ if (content instanceof Components.interfaces.nsIInterfaceRequestor)
>+ content.getInterface(Components.interfaces.nsIDOMWindowUtils).focus(null);
This doesn't work e.g. in a frame; the window you need to use is the one containing the element i.e. element.ownerDocument.defaultView
Attachment #271832 -
Flags: review?(neil) → review-
Assignee | ||
Comment 40•17 years ago
|
||
> I guess you're doing this as a quick shortcut?
Yes, but html elements seem to like it better.
Attachment #271832 -
Attachment is obsolete: true
Attachment #272020 -
Flags: review?(neil)
Comment 41•17 years ago
|
||
Comment on attachment 272020 [details] [diff] [review]
address issues
>+ var content = window.content;
Nit: you forgot to change this one too.
You might also want to change the variable name ;-)
Attachment #272020 -
Flags: review?(neil) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #272020 -
Flags: superreview?(bzbarsky)
Reporter | ||
Comment 42•17 years ago
|
||
Comment on attachment 272020 [details] [diff] [review]
address issues
>Index: dom/public/idl/base/nsIDOMWindowUtils.idl
>+ * @param aElement the element to focus
Document that this should be an element that comes from the document in question?
>Index: dom/src/base/nsDOMWindowUtils.cpp
>+ pc->EventStateManager()->ChangeFocusWith(content,
>+ nsIEventStateManager::eEventFocusedByApplication);
Is there a problem if the node is not actually in the document? If so, check here and bail?
>Index: toolkit/content/widgets/tabbrowser.xml
Does seamonkey need a similar change? If so, file it, please?
>+ if ("blur" in elem) {
>+ elem.blur();
I think you want to explicitly check for the right interfaces here; otherwise you could be calling some content-defined blur function, no?
Not like this wasn't a problem before...
Same thing for focus.
sr=bzbarsky with those nits.
Attachment #272020 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 43•17 years ago
|
||
> Does seamonkey need a similar change? If so, file it, please?
That's bug 323806
Attachment #272072 -
Flags: superreview?(bzbarsky)
Reporter | ||
Comment 44•17 years ago
|
||
Comment on attachment 272072 [details] [diff] [review]
fix comments
Looks great!
Attachment #272072 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 45•17 years ago
|
||
I would reopen right away, but...
https://bugzilla.mozilla.org/attachment.cgi?id=202809
the elements on this attachments no longer accept focus.
older versions show changes in fill when navigating with keyboard.
I should probably provide css versions that don't rely on script :-(
http://www.peepo.co.uk appears unchanged to #35
Comment 46•17 years ago
|
||
(In reply to comment #45)
>https://bugzilla.mozilla.org/attachment.cgi?id=202809
>the elements on this attachments no longer accept focus.
That looks like a recent regression - it works in a build I have from 29th May. Would you mind filing a new bug for that? Thanks.
Comment 47•17 years ago
|
||
#46
Neil
please can you confirm whether the issue in #35 is resolved?
Comment 48•17 years ago
|
||
(In reply to comment #47)
>please can you confirm whether the issue in #35 is resolved?
The issue in #35 should be resolved in the latest Firefox nightlies.
Comment 49•17 years ago
|
||
#48
the issue in #35 is at best only part fixed.
open http://www.peepo.co.uk in one tab
use keyboard to navigate through say 4 links.
open another tab http://www.peepo.co.uk
return to the first tab, and it appears that focus is retained.
however if instead one navigates through the second tab before returning the situation is once again a mess.
I propose to reopen this bug unless anyone has convincing arguments and test cases which demonstrate that keyboard navigation across tabs is fixed.
cheers
also please note in the absence of other evidence that #46 "looks like a recent regression"
is this not likely to be related to patches on this bug?
and if so may it be a bad idea to open another bug?
Comment 50•17 years ago
|
||
(In reply to comment #49)
>however if instead one navigates through the second tab before returning the
>situation is once again a mess.
Although I'm not using an official build I don't see any issues on this page.
>also please note in the absence of other evidence that #46 "looks like a recent
>regression"
>is this not likely to be related to patches on this bug?
Absolutely no relation at all.
Comment 51•17 years ago
|
||
#50
Neil OS X using most recent build from here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
more explicitely:
open http://www.peepo.co.uk in one tab
use keyboard to navigate through say 4 links.
open another tab http://www.peepo.co.uk
use keyboard to navigate through say 4 links.
return to the first tab, and it appears that focus is retained.
BUT now try to move focus...
in fact focus is with the application, not in the content window
and when focus moves to content window it starts with first link.
This contrasts with the first case in #49 where focus is in the content window on the link that had focus last.
Comment 52•17 years ago
|
||
#46
Neil,
Can this bug be fixed?
isn't it dependent on the bug that you are asking me to file.
if this bug is fixed does that mean keyboard navigation should behave in a predicatable and expected manner?
Perhaps I'm involuntarily expanding the scope of this bug, apologies.
Assignee | ||
Comment 53•17 years ago
|
||
When one navigates by using the keyboard to switch tabs, the focus is retained, but when clicking the tab labels, focus is switched to the window in the tab. I don't know why this is the behaviour used, but I didn't change it.
This bug is about tab switching while a non-html-or-xul element (such as svg) is focused, and that is fixed.
Comment 54•17 years ago
|
||
Neil Deakin,
Neil wrote in comment #46:
"Each window remembers which
element last had focus so that when you switch window it automatically restores
focus to that element."
this is not the behaviour at present.
as you appear not to be considering this issue in this bug I am reverting comments to my original bug which whilst marked as a dupe remains open.
cheers
Comment 55•17 years ago
|
||
#49
these outstanding issues have been moved to:
Bug 388146
Comment 56•17 years ago
|
||
(In reply to comment #53)
>When one navigates by using the keyboard to switch tabs, the focus is retained,
>but when clicking the tab labels, focus is switched to the window in the tab. I
>don't know why this is the behaviour used, but I didn't change it.
Obviously SeaMonkey behaviour (with the latest patch in bug 323806) is different, because I always see the focus being retained, except by design when switching tabs by focusing the tab and using the arrow keys.
>This bug is about tab switching while a non-html-or-xul element (such as svg)
>is focused, and that is fixed.
Agreed.
You need to log in
before you can comment on or make changes to this bug.
Description
•