Closed Bug 1357646 Opened 3 years ago Closed 3 years ago

Stop setting mType members in ParseAttribute methods

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

We fixed this in HTMLInputElement, but still have this in some other elements.  And some of those get it wrong too: they don't update mType on attr removals.
ParseAttribute should generally not mutate the element.
Attachment #8859453 - Flags: review?(michael)
This also fixes mType for cases in which the type attribute is removed.
Attachment #8859454 - Flags: review?(michael)
This also fixes mType for cases in which the type attribute is removed.
Attachment #8859455 - Flags: review?(michael)
Attachment #8859453 - Flags: review?(michael) → review+
Comment on attachment 8859454 [details] [diff] [review]
part 2.  Move the setting of mType from HTMLMenuElement::ParseAttribute to HTMLMenuElement::AfterSetAttr

Review of attachment 8859454 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMenuElement.cpp
@@ +117,5 @@
> +HTMLMenuElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
> +                              const nsAttrValue* aValue, bool aNotify)
> +{
> +  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::type) {
> +    if (!aValue) {

Can you flip the branches and remove the negation?
Attachment #8859454 - Flags: review?(michael) → review+
Comment on attachment 8859453 [details] [diff] [review]
part 1.  Move the setting of mType from HTMLButtonElement::ParseAttribute to HTMLButtonElement::AfterSetAttr

Review of attachment 8859453 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLButtonElement.cpp
@@ +431,5 @@
>                                  const nsAttrValue* aValue, bool aNotify)
>  {
>    if (aNameSpaceID == kNameSpaceID_None) {
>      if (aName == nsGkAtoms::type) {
>        if (!aValue) {

Actually, could you flip this if's condition as well?
> Actually, could you flip this if's condition as well?

Yes, done for all three.
Comment on attachment 8859455 [details] [diff] [review]
part 3.  Move the setting of mType from HTMLMenuItemElement::ParseAttribute to HTMLMenuItemElement::AfterSetAttr

Review of attachment 8859455 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMenuItemElement.cpp
@@ +378,5 @@
>    if (aNameSpaceID == kNameSpaceID_None) {
> +    // Handle type changes first, since some of the later conditions in this
> +    // method look at mType and want to see the new value.
> +    if (aName == nsGkAtoms::type) {
> +      if (!aValue) {

Here too.
Attachment #8859455 - Flags: review?(michael) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8d9ca30319
part 1.  Move the setting of mType from HTMLButtonElement::ParseAttribute to HTMLButtonElement::AfterSetAttr.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8904e0ad25
part 2.  Move the setting of mType from HTMLMenuElement::ParseAttribute to HTMLMenuElement::AfterSetAttr.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d447189d4d
part 3.  Move the setting of mType from HTMLMenuItemElement::ParseAttribute to HTMLMenuItemElement::AfterSetAttr.  r=mystor
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.