Closed Bug 586763 Opened 14 years ago Closed 14 years ago

Several integer reflected attributes have wrong defaults

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b7

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(2 files, 2 obsolete files)

Patch v3, conflicts fixed
13.82 KB, patch
sicking
: review+
sicking
: approval2.0+
Details | Diff | Splinter Review
Patch v2-v3 interdiff
4.24 KB, patch
Details | Diff | Splinter Review
""" If a reflecting IDL attribute is a signed integer type (long) then, on getting, . . . if the attribute is absent, then the default value must be returned instead, or 0 if there is no default value. """ http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes """ If the start attribute is present, user agents must parse it as an integer, in order to determine the attribute's value. The default value, used if the attribute is missing or if the value cannot be converted to a number according to the referenced algorithm, is 1 if the element has no reversed attribute, and is the number of child li elements otherwise. """ http://www.whatwg.org/specs/web-apps/current-work/multipage/grouping-content.html#the-ol-element No default is mentioned for li.value. Thus the following test case: <!doctype html> <script> var ol = document.createElement("ol"); var li = document.createElement("li"); alert(ol.start + " " + li.value); </script> should alert "1 0". It does on IE8, Chrome dev, Safari 5, and Opera 10.60. On Firefox 4.0b3, it alerts "-1 -1".
OS: Linux → All
Hardware: x86 → All
Seems like maybe we need something like: - NS_IMPL_INT_ATTR should be defined to NS_IMPL_INT_ATTR_DEFAULT_VALUE(..., 0) instead of (..., -1) - in nsHTMLOListElement.cpp, NS_IMPL_INT_ATTR(nsHTMLSharedListElement, Start, start) should be using NS_IMPL_INT_ATTR_DEFAULT_VALUE but if we're going to change NS_IMPL_INT_ATTR, we probably want to audit all its users to look for others that should be using NS_IMPL_INT_ATTR_DEFAULT_VALUE
Attached patch Patch v1 (no tests yet) (obsolete) — — Splinter Review
Analysis of current NS_IMPL_INT_ATTR() callers (comparing to IE8, Chrome dev, Opera 10.60, Safari 5): * tabIndex is supposed to sometimes be -1 and sometimes 0, so the semantics of those should be kept intact. * object.hspace is 0 by default in all other browsers, -1 in Firefox. Should change to the new default. vspace presumably the same. * img.hspace also 0 by default in other browsers. I'll assume this is true for all vspace/hspace. * video.width is 0 where defined, undefined in Safari 5 and IE8. (No idea why in Safari 5.) Should change to the new default along with video.height (which I assume is the same story). * li.value, already discussed. * pre.width is -1 in Firefox, empty string in IE8, and 0 in everyone else. Should change to the new default. * textarea.cols is 20 in IE and WebKit, -1 in Firefox, 0 in Opera. Spec agrees with Opera, but IE/WebKit makes more sense plus has much larger market share, so we should switch to that and file a spec bug. * textarea.rows is 2 in IE and WebKit, -1 in Firefox, 0 in Opera. Same as previous. The basic test case I used was <!doctype html> <script> alert(document.createElement("video").width); </script> adapted as appropriate. Attached patch compiles, and brief manual testing indicates it does what I intended, but I haven't written tests yet, nor run existing tests to check for regressions. Who should I ask for review/super-review? For tests, I guess it's enough to just do lots of stuff like is(document.createElement("li").value, 0, "Reflected attribute li.value should default to 0"), if there's no existing test that this causes to fail that I can fix to test it?
Assignee: nobody → Simetrical+ff
Status: NEW → ASSIGNED
Maybe ask :jst or :sicking for review (unless somebody more knowledgable suggests somebody else).
Revising summary to cover all integer reflected attributes with incorrect defaults.
Summary: Reflected attributes ol.start and li.value have wrong defaults → Several integer reflected attributes have wrong defaults
Just to clarify: the only place where the behavior in my patch doesn't match spec, AFAICT, is textarea.cols and textarea.rows, where I think the spec makes less sense than the IE/WebKit behavior and should be changed (and I'll file a spec bug on that).
Attached patch Patch v2, with tests (obsolete) — — Splinter Review
This patch is ready for review, I guess. Rationale for the exact changes is given in comment 2 and comment 5. The tests are very minimal, but I can't think what else to add. I or someone else should do another pass over the NS_IMPL_INT_ATTR and NS_IMPL_INT_ATTR_DEFAULT_VALUE users to see if anything is missing. I filed a spec bug about textarea.rows/cols: http://www.w3.org/Bugs/Public/show_bug.cgi?id=10666
Attachment #476670 - Attachment is obsolete: true
Attachment #476988 - Flags: review?
Also see bug 586126 for why some of these are long when the spec says unsigned long, if you're comparing the changes to the spec yourself.
Comment on attachment 476988 [details] [diff] [review] Patch v2, with tests jonas agreed to review this when I asked him in #whatwg.
Attachment #476988 - Flags: review?(jst) → review?(jonas)
Comment on attachment 476988 [details] [diff] [review] Patch v2, with tests Jonas suggested :jst for super-review.
Attachment #476988 - Flags: superreview?(jst)
Attachment #476988 - Flags: superreview?(jst) → superreview+
Attachment #476988 - Flags: approval2.0?
Whiteboard: [can land]
patching file content/html/content/src/nsHTMLObjectElement.cpp Hunk #1 FAILED at 369 1 out of 1 hunks FAILED -- saving rejects to file content/html/content/src/nsHTMLObjectElement.cpp.rej patching file content/html/content/test/Makefile.in Hunk #1 FAILED at 229 1 out of 1 hunks FAILED -- saving rejects to file content/html/content/test/Makefile.in.rej
Keywords: checkin-needed
Whiteboard: [can land]
Same as previous patch, except that the change to nsHTMLObjectElement.cpp is dropped -- the new code (from bug 596350) sets the default sometimes to 0 and sometimes to -1, so it has to use NS_IMPL_INT_ATTR_DEFAULT_VALUE() anyway. The old code set it unconditionally to 0, so I switched it to use NS_IMPL_INT_ATTR(), but that no longer works. Plus of course I fixed the trivial Makefile.in conflict.
Attachment #476988 - Attachment is obsolete: true
Attachment #482648 - Flags: superreview?(jst)
Attachment #482648 - Flags: review?(jonas)
Ugh, when a patch has already been reviewed, please include interdiffs so that reviewers don't have to go through the whole patch again.
Especially when your comment doesn't match the change you did. There is no nsHTMLObjectElement.cpp changes in the patch ;)
Here's the interdiff. As I said, there are no changes to nsHTMLObjectElement.cpp in the new patch because they were obsoleted by the patch for bug 596350. That changed -NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLObjectElement, TabIndex, tabindex, 0) +NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLObjectElement, TabIndex, tabindex, + IsFocusableForTabIndex() ? 0 : -1) so my change in the original patch -NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLObjectElement, TabIndex, tabindex, 0) +NS_IMPL_INT_ATTR(nsHTMLObjectElement, TabIndex, tabindex) no longer makes sense/is no longer necessary.
Comment on attachment 482648 [details] [diff] [review] Patch v3, conflicts fixed Thanks for confirming! For the record, an interdiff isn't a diff-of-diffs. As I'm sure you can tell, diffing diffs tend to generate very hard-to-read results. An interdiff is a "real" diff that takes you from one version of the patch to another. Anyhow, r=me. I also don't think you need to re-request super review unless you really want to. I'll mark a=me either way.
Attachment #482648 - Flags: superreview?(jst)
Attachment #482648 - Flags: review?(jonas)
Attachment #482648 - Flags: review+
Attachment #482648 - Flags: approval2.0+
Checked in. Thanks for the patch!! http://hg.mozilla.org/mozilla-central/rev/78b7ea52d52e For future reference, it's great if you can include username and checkin comment in the attached patch when you're asking someone else to check in. You can create a patch with this info by using |hg export|.
Actually, this turned the tree orange :( The merge to tip was wrong, <object>.tabIndex defaults to -1 now, but the test was expecting 0. Fix checked in: http://hg.mozilla.org/mozilla-central/rev/cb8754333dd5
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I do not understand why these changes in tabindex? We are changing our behavior but still not following the specifications.
(In reply to comment #18) > I do not understand why these changes in tabindex? We are changing our behavior > but still not following the specifications. We aren't changing our behaviour as far as I can tell: the patch changes the default in NS_IMPL_INT_ATTR and replaces its uses not to change behaviour. However, it appears to me that we do follow the specification: > The tabIndex IDL attribute must reflect the value of the tabindex content > attribute. Its default value is 0 for elements that are focusable and −1 for > elements that are not focusable.
(In reply to comment #16) > For future reference, it's great if you can include username and checkin > comment in the attached patch when you're asking someone else to check in. You > can create a patch with this info by using |hg export|. Okay, I'll do that in the future. (In reply to comment #17) > Actually, this turned the tree orange :( > > The merge to tip was wrong, <object>.tabIndex defaults to -1 now, but the test > was expecting 0. > > Fix checked in: > http://hg.mozilla.org/mozilla-central/rev/cb8754333dd5 Oops, I should have rerun the tests for the new patch version. Thanks for the fix. (In reply to comment #18) > I do not understand why these changes in tabindex? We are changing our behavior > but still not following the specifications. This shouldn't have changed tabindex behavior -- it should only be changing the things mentioned in comment 2. Specifically: hspace, vspace, video.width, video.height, li.value, pre.width, textarea.cols, textarea.rows, ol.start. All these were changed to match the current spec, plus in all cases at least one other browser. (E.g., Chrome matches the new behavior in all cases.)
(In reply to comment #19) > (In reply to comment #18) > > I do not understand why these changes in tabindex? We are changing our behavior > > but still not following the specifications. > > We aren't changing our behaviour as far as I can tell: the patch changes the > default in NS_IMPL_INT_ATTR and replaces its uses not to change behaviour. Isn't changing the default value for .tabIndex changing the behavior of the IDL attribute? > However, it appears to me that we do follow the specification: > > > The tabIndex IDL attribute must reflect the value of the tabindex content > > attribute. Its default value is 0 for elements that are focusable and −1 for > > elements that are not focusable. I don't think so. <input disabled> should have .tabIndex returning -1 for example. Which isn't the case with this patch AFAICT.
(In reply to comment #21) > Isn't changing the default value for .tabIndex changing the behavior of the IDL > attribute? I didn't change the default value for anything in the original patch, AFAICT. It might be that my original patch didn't merge correctly with intervening changes, though. I should have double-checked all the NS_IMPL_INT_ATTR callers again when writing the new patch version. > I don't think so. <input disabled> should have .tabIndex returning -1 for > example. Which isn't the case with this patch AFAICT. Did the patch actually change this? As far as I can tell with a quick glance at the code, that returned 0 both before and after the patch (although I didn't compile and test to see).
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: