Bug 161698 – Align HTMLAreaElement shape parsing with the specification

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Align HTMLAreaElement shape parsing with the specification:
- https://html.spec.whatwg.org/#attr-area-shape

This also aligns our behavior with Chrome.

[Chris Dumez]

Created attachment 288156 [details]
Patch

[Daniel Bates]

Comment on attachment 288156 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288156&action=review

> Source/WebCore/ChangeLog:12
> +        In particular, add support for circ / polygon non conforming shapes
> +        and use rectangle as default state.

How did you come to the decision to add support for such "non conforming" shapes? I know that adding such support makes us pass more tests and matches Chrome behavior. Should non-conforming shapes be recognized in standards mode (as opposed to quirks mode)? Does this agree with other browsers, including Firefox and the latest IE/Edge? Are we expected to add support for such non-conforming shapes? I am unclear of our obligation to implement such non-conforming shapes. The wording in "Errors involving limits that have been imposed merely to simplify the language" of section "Restrictions on content models and on attribute values" of the HTML living standard, <https://html.spec.whatwg.org/#restrictions-on-content-models-and-on-attribute-values>, seems to imply that we should support "circ" because it is easier to teach?

As far as I can tell circ, polygon, rectangle are Microsoft extensions that are documented at <https://msdn.microsoft.com/en-us/library/ms534628(v=vs.85).aspx>.

> Source/WebCore/html/HTMLAreaElement.cpp:64
> -        else if (equalLettersIgnoringASCIICase(value, "rect"))
> +        else {
> +            // The missing value default is the rectangle state.
>              m_shape = Rect;
> +        }

Please add tests for shape "rectangle" and the "missing value" case.

[Daniel Bates]

Comment on attachment 288156 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288156&action=review

>> Source/WebCore/html/HTMLAreaElement.cpp:64
>> +        }
> 
> Please add tests for shape "rectangle" and the "missing value" case.

Disregard the remark about adding a test for the "missing value" case. Please also add a test for shape "polygon".

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 288156 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288156&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        In particular, add support for circ / polygon non conforming shapes
> > +        and use rectangle as default state.
> 
> How did you come to the decision to add support for such "non conforming"
> shapes? I know that adding such support makes us pass more tests and matches
> Chrome behavior. Should non-conforming shapes be recognized in standards
> mode (as opposed to quirks mode)? Does this agree with other browsers,
> including Firefox and the latest IE/Edge? Are we expected to add support for
> such non-conforming shapes? I am unclear of our obligation to implement such
> non-conforming shapes. The wording in "Errors involving limits that have
> been imposed merely to simplify the language" of section "Restrictions on
> content models and on attribute values" of the HTML living standard,
> <https://html.spec.whatwg.org/#restrictions-on-content-models-and-on-
> attribute-values>, seems to imply that we should support "circ" because it
> is easier to teach?

Because:
1. It is in the HTML spec
2. It is covered by W3C tests
3. It is supported by Chrome.

> 
> As far as I can tell circ, polygon, rectangle are Microsoft extensions that
> are documented at
> <https://msdn.microsoft.com/en-us/library/ms534628(v=vs.85).aspx>.
> 
> > Source/WebCore/html/HTMLAreaElement.cpp:64
> > -        else if (equalLettersIgnoringASCIICase(value, "rect"))
> > +        else {
> > +            // The missing value default is the rectangle state.
> >              m_shape = Rect;
> > +        }
> 
> Please add tests for shape "rectangle" and the "missing value" case.

Ok, I'll extend testing.

[Chris Dumez]

(In reply to comment #4)
> (In reply to comment #2)
> > Comment on attachment 288156 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=288156&action=review
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        In particular, add support for circ / polygon non conforming shapes
> > > +        and use rectangle as default state.
> > 
> > How did you come to the decision to add support for such "non conforming"
> > shapes? I know that adding such support makes us pass more tests and matches
> > Chrome behavior. Should non-conforming shapes be recognized in standards
> > mode (as opposed to quirks mode)? Does this agree with other browsers,
> > including Firefox and the latest IE/Edge? Are we expected to add support for
> > such non-conforming shapes? I am unclear of our obligation to implement such
> > non-conforming shapes. The wording in "Errors involving limits that have
> > been imposed merely to simplify the language" of section "Restrictions on
> > content models and on attribute values" of the HTML living standard,
> > <https://html.spec.whatwg.org/#restrictions-on-content-models-and-on-
> > attribute-values>, seems to imply that we should support "circ" because it
> > is easier to teach?
> 
> Because:
> 1. It is in the HTML spec
> 2. It is covered by W3C tests
> 3. It is supported by Chrome.
> 
> > 
> > As far as I can tell circ, polygon, rectangle are Microsoft extensions that
> > are documented at
> > <https://msdn.microsoft.com/en-us/library/ms534628(v=vs.85).aspx>.
> > 
> > > Source/WebCore/html/HTMLAreaElement.cpp:64
> > > -        else if (equalLettersIgnoringASCIICase(value, "rect"))
> > > +        else {
> > > +            // The missing value default is the rectangle state.
> > >              m_shape = Rect;
> > > +        }
> > 
> > Please add tests for shape "rectangle" and the "missing value" case.
> 
> Ok, I'll extend testing.

Actually, the "missing value" case is also covered in the test I rebaselined. "rectangle" is not tested but even if not supposed it would fallback to using Rect shape by default.

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > Comment on attachment 288156 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=288156&action=review
> > > 
> > > > Source/WebCore/ChangeLog:12
> > > > +        In particular, add support for circ / polygon non conforming shapes
> > > > +        and use rectangle as default state.
> > > 
> > > How did you come to the decision to add support for such "non conforming"
> > > shapes? I know that adding such support makes us pass more tests and matches
> > > Chrome behavior. Should non-conforming shapes be recognized in standards
> > > mode (as opposed to quirks mode)? Does this agree with other browsers,
> > > including Firefox and the latest IE/Edge? Are we expected to add support for
> > > such non-conforming shapes? I am unclear of our obligation to implement such
> > > non-conforming shapes. The wording in "Errors involving limits that have
> > > been imposed merely to simplify the language" of section "Restrictions on
> > > content models and on attribute values" of the HTML living standard,
> > > <https://html.spec.whatwg.org/#restrictions-on-content-models-and-on-
> > > attribute-values>, seems to imply that we should support "circ" because it
> > > is easier to teach?
> > 
> > Because:
> > 1. It is in the HTML spec
> > 2. It is covered by W3C tests
> > 3. It is supported by Chrome.
> > 
> > > 
> > > As far as I can tell circ, polygon, rectangle are Microsoft extensions that
> > > are documented at
> > > <https://msdn.microsoft.com/en-us/library/ms534628(v=vs.85).aspx>.
> > > 
> > > > Source/WebCore/html/HTMLAreaElement.cpp:64
> > > > -        else if (equalLettersIgnoringASCIICase(value, "rect"))
> > > > +        else {
> > > > +            // The missing value default is the rectangle state.
> > > >              m_shape = Rect;
> > > > +        }
> > > 
> > > Please add tests for shape "rectangle" and the "missing value" case.
> > 
> > Ok, I'll extend testing.
> 
> Actually, the "missing value" case is also covered in the test I
> rebaselined. "rectangle" is not tested but even if not supposed it would
> fallback to using Rect shape by default.

if not *supported*.

As a matter of fact, my code change does not explicitly test for "rectangle".

[Chris Dumez]

Created attachment 288170 [details]
Patch

[Daniel Bates]

Comment on attachment 288170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288170&action=review

> Source/WebCore/ChangeLog:14
> +        This also aligns our behavior with Chrome.

A more compelling argument is that "circ", "polygon" and "rectangle" have appeared in various publications, including in O'Reilly's HTML & XHTML: The Definitive Guide.

[Chris Dumez]

Comment on attachment 288170 [details]
Patch

Clearing flags on attachment: 288170

Committed r205562: <http://trac.webkit.org/changeset/205562>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.