Closed Bug 596350 Opened 15 years ago Closed 15 years ago

<object> should not be focusable by default

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b7
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mounir, Assigned: mounir)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

Testcase
1.86 KB, text/html
Details
Patch v1.3
14.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
According to the specs, they are not interactive elements by default (only with usemap). Looks like Opera and Chromium do not allow focus on object by default. At least, no focus event is sent.
Considering that is a regression this seems to be a blocker candidate.
blocking2.0: --- → ?
So what is the regression?
Oups, I forgot the test case. See the URL. With 3.6 nothing should happen. The same with Opera or Chromium. With trunk, you will see FAIL!. I think this came with the big focus change. I will investigate that after the code freeze.
URL: data:text/html,<object onfocus="docum...
Assignee: nobody → mounir.lamouri
blocking2.0: ? → betaN+
This has been introduced by bug 554635 which basically did only that. I think we should revert it.
Status: NEW → ASSIGNED
Blocks: 554635
Blocks: 569315
bug 554635 is a regression so if you back it out you're just swapping one regression for another.
I agree with Mounir, we should backout bug 554635 (and reopen it). I also find the current implementation of nsHTMLObjectElement::IsDisabled() slightly odd (it just returns PR_FALSE). If the intention is to avoid being affected by a disabled ancestor fieldset I think it should "return nsGenericHTMLElement::IsDisabled()" instead so that <object disabled tabindex=0> behaves like <div disabled tabindex=0> (ie. not tabbable)
Neil, see comment 4 and comment 6? Mounir, do the other browsers make <object> containing HTML or SVG not focusable too? I'd think that would be focusable....
(In reply to comment #7) > Neil, see comment 4 and comment 6? > > Mounir, do the other browsers make <object> containing HTML or SVG not > focusable too? I'd think that would be focusable.... When a child <object> is focused, Webkit makes the parent.activeElement be the parent's <body>. Webkit does the same for <iframe>. Firefox and IE (I tested IE8) makes the parent.activeElement be the <object> or <iframe>. I don't agree that it should be backed out as I don't see why we should regress a bug which makes it so that focus works in <object data="some_web_page"> and instead fixes an obscure issue where an empty <object> is focusable. Instead, we should just fix the latter bug.
Webkit's behavior doesn't make any sense. And yeah, object containing a document should work like iframe.
Attached file subdoc with a button (obsolete) —
Attached file Testcase —
Attachment #477536 - Attachment is obsolete: true
(In reply to comment #8) > When a child <object> is focused, Webkit makes the parent.activeElement be the > parent's <body>. Webkit does the same for <iframe>. That's not what I'm seeing. I tested a current nightly Webkit on OSX, Chrome 6 and 7 on Linux, and got the same results: focus() is ignored for <object> AND <iframe> unless there is a specified tabindex. If it has a tabindex, focus() changes activeElement to that element. Tabbing through the elements ignores the <object>/<iframe> unless there is a tabindex. > Firefox and IE (I tested IE8) makes the parent.activeElement be the <object> or > <iframe>. True for IE8 and Firefox trunk. That's the bug. > I don't agree that it should be backed out as I don't see why we should regress > a bug which makes it so that focus works in <object data="some_web_page"> and > instead fixes an obscure issue where an empty <object> is focusable. It's not just an obscure issue with focus()-ing empty <object>s. The problem is that a default tabindex=0 affects tabbing.
(In reply to comment #12) > (In reply to comment #8) > > When a child <object> is focused, Webkit makes the parent.activeElement be the > > parent's <body>. Webkit does the same for <iframe>. > > That's not what I'm seeing. I tested a current nightly Webkit on OSX, > Chrome 6 and 7 on Linux, and got the same results: focus() is ignored > for <object> AND <iframe> unless there is a specified tabindex. > If it has a tabindex, focus() changes activeElement to that element. > Tabbing through the elements ignores the <object>/<iframe> unless there > is a tabindex. I read Neil's message so that if something inside the object.contentDocument is focused, then parent.activeElement in webkit is parent.body. Nothing to do with focus(); > > Firefox and IE (I tested IE8) makes the parent.activeElement be the <object> or > > <iframe>. > > True for IE8 and Firefox trunk. That's the bug. Why is that a bug in Gecko and not in HTML5 draft?
(In reply to comment #9) > And yeah, object containing a document should work like iframe. That's not what HTML5 currently says. It says <iframe> and <embed> are "interactive content": http://www.whatwg.org/specs/web-apps/current-work/#interactive-content-0 but <object> is only interactive when it has a usemap attribute. (In reply to comment #13) > Why is that a bug in Gecko and not in HTML5 draft? Gecko, Webkit and Presto agrees with the HTML5 spec that tabbing should not stop at <object> (before this regression that is). If we think the inconsistency between object/iframe, or the HTML5 spec, doesn't make sense, I would much rather see that Gecko didn't stop at iframe either - unless the author specified a tabindex. (similar to Webkit/Presto, although they seem buggy in some cases)
> That's not what HTML5 currently says. It's possible that HTML5 is wrong, for the object-containing-document case... Then again, object-containing-svg is more like image-containing-svg in various ways than it's like iframe-containing-svg. So it's a tossup.
A simple testcase http://mozilla.pettay.fi/moztests/tabbing.html IMO, one should be able to tab to the <input> element in the <object>. Chrome doesn't allow that. Opera is too broken to even test tabbing. I don't have Windows running atm to test IE. m-c allows focusing the input inside object.
(In reply to comment #5) > bug 554635 is a regression so if you back it out you're just swapping one > regression for another. bug 554635 is not a real regression. The website doesn't work with 3.6. As mentioned by the reporter, it worked at some point with nightlies and suddenly stopped to work. This regression is a regression between 3.6 and 4.0. I think that's much more important. (In reply to comment #6) > I agree with Mounir, we should backout bug 554635 (and reopen it). > > I also find the current implementation of nsHTMLObjectElement::IsDisabled() > slightly odd (it just returns PR_FALSE). The object element can't be disabled. At least, according to the specifications. If there is any backward compatibility involving here we should change that and ask the specifications to change. I strongly think we should revert the change. The fix sounds completely inappropriate. A more appropriate fix would have been to make the object element focusable when data is set or even better when the data inside might need focus.
(In reply to comment #17) > > bug 554635 is not a real regression. The website doesn't work with 3.6. As > mentioned by the reporter, it worked at some point with nightlies and suddenly > stopped to work. This regression is a regression between 3.6 and 4.0. I think > that's much more important. I read that as the reporter indicating that it worked in 3.5. Can anyone confirm whether it does or not.
What I don't understand is why you think the solution is to: 1. back out the fix for 554635. 2. fix the bug in a manner you haven't yet described. I propose instead: 1. Describe and implement a fix that works for all <objects>. The patch in bug 569315 seemed like it was on right track. To be clear, the tab behaviour I would expect from the testcase (https://bugzilla.mozilla.org/attachment.cgi?id=477537) is: 1. Tab to the button labeled 'object-alt-button' 2. Tab to the Google image 3. Tab to the document beside it with the red outline. 4. Tab to the button inside it. 5. Tab to the document in the iframe. 6. Tab to the button inside the iframe. 7. Tab through the next set in a similar manner, but first also include the first <object> with the content inside it.
Specifically, I think bug 569315 should be fixed and this bug marked wontfix.
Attached patch Patch v1 (obsolete) — — Splinter Review
I think this patch should make everyone happy: object containing documents, like object containing plugins are now focusable. Otherwise, the object has to contain a tabindex attribute to be focusable.
Attachment #477763 - Flags: review?(Olli.Pettay)
Comment on attachment 477763 [details] [diff] [review] Patch v1 >-NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLObjectElement, TabIndex, tabindex, 0) >+//NS_IMPL_INT_ATTR(nsHTMLObjectElement, TabIndex, tabindex) >+NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLObjectElement, TabIndex, tabindex, >+ (Type() == eType_Plugin || Type() == eType_Document) ? 1 : 0) Why you left the line starting with //? This needs some tests. And Neil should look at this too.
Attachment #477763 - Flags: review?(enndeakin)
Attachment #477763 - Flags: review?(Olli.Pettay)
Attachment #477763 - Flags: review+
Comment on attachment 477763 [details] [diff] [review] Patch v1 Having a default tabindex=1 is clearly wrong. (try tabbing through <button><object><button>)
Attachment #477763 - Flags: review-
Attached patch fix (obsolete) — — Splinter Review
Here's what I suggest. It backs out bug 554635 and thus restores tabindex=-1 when not specified. The added code makes <object> focusable by mouse or script when there is a sub-document (of any kind). It fixes the original problem in bug 554635, without making <object> tabbable by default. It makes us behave very closely to what Webkit does.
Oups, that was not my intention, it's a mistake.
Attached patch Patch v1.1 (obsolete) — — Splinter Review
Mats, sorry for the mistake, the default tabindex value should be 0 if it's focusable and -1 otherwise. That's following the specifications. The only difference with the specifications here is <object> are focusable if they contain a plugin or a document which seems to be a good idea. Olli, I will write test cases for that.
Attachment #477763 - Attachment is obsolete: true
Attachment #477978 - Flags: review?(enndeakin)
Attachment #477763 - Flags: review?(enndeakin)
Attachment #477978 - Flags: review?(matspal)
Comment on attachment 477978 [details] [diff] [review] Patch v1.1 >- if (Type() == eType_Plugin) { >+ // This method doesn't call nsGenericHTMLFormElement intentionally. >+ // TODO: It should probably be changed when bug 597242 will be fixed. >+ if (Type() == eType_Plugin || Type() == eType_Document) { > // Has plugin content: let the plugin decide what to do in terms of > // internal focus from mouse clicks > if (aTabIndex) { >- GetTabIndex(aTabIndex); >+ GetIntAttr(nsGkAtoms::tabindex, 0, aTabIndex); I'd just leave this as a call to GetTabIndex to avoid having the default value specified in two places. I've not decided yet if we want to allow image documents to be in the tab order by default but that can be a separate issue.
Attachment #477978 - Flags: review?(enndeakin) → review+
(In reply to comment #24) > Created attachment 477976 [details] [diff] [review] > fix > > Here's what I suggest. It backs out bug 554635 and thus restores > tabindex=-1 when not specified. > The added code makes <object> focusable by mouse or script when there > is a sub-document (of any kind). It fixes the original problem in > bug 554635, without making <object> tabbable by default. This patch causes <object data="page.html"> and its contents to be skipped in the tab order from my testing, which is not correct.
Comment on attachment 477978 [details] [diff] [review] Patch v1.1 I don't believe <object> should be tabbable without an explicit tabindex. HTML5 clearly says it's non-interactive, and that's how other UAs behave, including older versions of Firefox. Since there is some agreement between spec and vendor implementations here I don't think we should change our behavior just because we think the new behavior is what "makes sense". If we want the spec to change we should do that first.
Attachment #477978 - Flags: review?(matspal) → review-
(In reply to comment #28) > This patch causes <object data="page.html"> and its contents to be skipped in > the tab order from my testing, which is not correct. I believe you are wrong. The HTML5 spec and other vendor implementations agree with me. See comment 14.
I'm going to bring that to whatwg mailing list and see what other implementors think.
Actually, according to the specifications [1], we can make anything focusable. Only of focusable elements are recommended. IOW, there is no need to bring that to the list, this should be fine. [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#focus-management
Comment on attachment 477978 [details] [diff] [review] Patch v1.1 Mat, I'm reverting you review per previous comment. If you still do not agree, please, bring that to the whatwg list.
Attachment #477978 - Flags: review-
Attached patch Patch v1.2 (obsolete) — — Splinter Review
This is adding tests and fixing a couple of things (contenteditable stuff). bug 597241 and bug 597242 should make this quite nicer. Olli, can you quickly re-check the patch?
Attachment #477976 - Attachment is obsolete: true
Attachment #477978 - Attachment is obsolete: true
Attachment #478957 - Flags: review?(Olli.Pettay)
Comment on attachment 478957 [details] [diff] [review] Patch v1.2 I was looking at the (i)frame focus handling, and noticed that is has special code for zombie documents. IMO <object>s containing documents should behave like <iframe>s. Could you perhaps add some some helper method to nsGenericHTMLElement which nsGenericHTMLFrameElement::IsHTMLFocusable and object could use. Sorry about not noticing that before.
Attachment #478957 - Flags: review?(Olli.Pettay) → review-
Attached patch Patch v1.3 — — Splinter Review
Attachment #478957 - Attachment is obsolete: true
Attachment #479141 - Flags: review?(Olli.Pettay)
Attachment #479141 - Flags: review?(Olli.Pettay) → review+
Pushed: http://hg.mozilla.org/mozilla-central/rev/f7c00c22da82
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Blocks: 284610
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Why does the test case here want <object data="data:text/html,foo"></object> to hit the return false; bit in nsCOMPtr<nsIContentViewer> contentViewer; docShell->GetContentViewer(getter_AddRefs(contentViewer)); if (!contentViewer) { return false; } in nsContentUtils?
Ping? Any ideas why <object data="data:text/html,foo"></object> has the test case expectation it has?
What would you expect?
(In reply to comment #40) > What would you expect? From the Web compat point of view, no idea. From the Gecko internals point of view, I expect us to move to a world where docshells always have a content viewer.
(In reply to comment #39) > Ping? Any ideas why <object data="data:text/html,foo"></object> has the test > case expectation it has? It seem that things become OK if I make the objects render (i.e. if I remove the display: none; div around them). Is it intentional that focus is being tested for objects that aren't being rendered? Is it intentional that display: none; affects the result?
OUPS! No, I don't think a non-rendered element should be focusable... This test is obviously wrong...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: