Closed
Bug 668821
Opened 14 years ago
Closed 13 years ago
HTMLPreElement width should be reflected as a long
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
DUPLICATE
of bug 824823
People
(Reporter: dzbarsky, Assigned: atulagrwl)
References
Details
Attachments
(1 file)
|
4.63 KB,
patch
|
mounir
:
review-
|
Details | Diff | Splinter Review |
|
Assignee | |
Comment 1•14 years ago
|
||
|
||
Comment 2•14 years ago
|
||
Comment on attachment 558080 [details] [diff] [review]
Patch v1
Review of attachment 558080 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/html/content/src/nsHTMLPreElement.cpp
@@ +62,5 @@
> NS_FORWARD_NSIDOMHTMLELEMENT(nsGenericHTMLElement::)
>
> // nsIDOMHTMLPreElement
> + NS_IMETHOD GetWidth(PRUint32* aWidth);
> + NS_IMETHOD SetWidth(PRUint32 aWidth);
Could you use NS_DECL_NSIDOMHTMLPREELEMENT instead?
@@ +107,5 @@
>
> NS_IMPL_ELEMENT_CLONE(nsHTMLPreElement)
>
>
> +NS_IMPL_UINT_ATTR(nsHTMLPreElement, Width, width)
I might have missed something but this attribute doesn't seem to be part of the specs. Why?
::: content/html/content/test/test_pre_attributes_refection.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=668821
> +-->
Remove this.
@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=668821
> +-->
> +<head>
> + <title>Test for Bug 668821</title>
Use something like "Tests for HTMLPreElement attributes reflection" as a title.
@@ +10,5 @@
> + <script type="application/javascript" src="reflect.js"></script>
> + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=668821">Mozilla Bug 668821</a>
Remove this.
@@ +15,5 @@
> +<p id="display"></p>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +/** Test for Bug 668821 **/
Use something like "Tests for HTMLPreElement attributes reflection" in the comment.
|
||
Comment 3•14 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) from comment #2)
> Comment on attachment 558080 [details] [diff] [review]
> @@ +107,5 @@
> >
> > NS_IMPL_ELEMENT_CLONE(nsHTMLPreElement)
> >
> >
> > +NS_IMPL_UINT_ATTR(nsHTMLPreElement, Width, width)
>
> I might have missed something but this attribute doesn't seem to be part of
> the specs. Why?
You did miss something: <http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.html#dom-pre-width>.
|
||
Comment 4•14 years ago
|
||
Comment on attachment 558080 [details] [diff] [review]
Patch v1
Mounir can review this, since he's already started.
Attachment #558080 -
Flags: review?(bzbarsky) → review?(mounir)
|
|
||
Comment 5•14 years ago
|
||
Comment on attachment 558080 [details] [diff] [review]
Patch v1
Review of attachment 558080 [details] [diff] [review]:
-----------------------------------------------------------------
Your patch is correct (with the previous nits fixed) but I think the specs are wrong here. pre.width is reflected as a long everywhere [1] except in Gecko. We should probably have Gecko's behavior changed and ask for a spec change.
[1] Tested in Presto (Opera 11.51), Webkit (Chromium 15) and IE6/9.
Attachment #558080 -
Flags: review?(mounir) → review-
|
|
||
Comment 6•14 years ago
|
||
Spec bug opened:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14090
|
|
Assignee | |
Comment 7•14 years ago
|
||
Agreed but a similar bug is closed in spec as 'Closed, Won't fix'.
(http://www.w3.org/Bugs/Public/show_bug.cgi?id=10345)
|
|
||
Comment 8•14 years ago
|
||
Not exactly the same issue. In our case, it's the fact that browsers don't do what the specs ask and -even more for an obsoleted feature- the specs want to match what is actually done.
|
|
Assignee | |
Comment 9•14 years ago
|
||
If that's the case, this will be fixed in spec. Good enough.
|
|
||
Comment 10•14 years ago
|
||
Note that if the specs change, we will have to change how pre.width behaves because right now it's a long but it is reflected as an unsigned long.
|
|
||
Updated•14 years ago
|
Summary: HTMLPreElement width should be unsigned long → HTMLPreElement width should be reflected as a long
|
|
||
Comment 11•14 years ago
|
||
The specs have been changed. Atul, do you still want to work on this? :)
|
|
Assignee | |
Comment 12•14 years ago
|
||
Yes, I would like to work on this but expect some delay. In the meantime if somebody else wants to pick this up, I won't mind ;)
|
Reporter | |
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
|
|
||
Comment 14•13 years ago
|
||
I don't undestand this bug. We reflected as long before, and we continue to do so... What was the bug about?
Shouldn't it have gotten resolved invalid right around comment 11?
You need to log in
before you can comment on or make changes to this bug.
Description
•