Stop setting mType members in ParseAttribute methods

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

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.
Created attachment 8859453 [details] [diff] [review]
part 1.  Move the setting of mType from HTMLButtonElement::ParseAttribute to HTMLButtonElement::AfterSetAttr

ParseAttribute should generally not mutate the element.
Attachment #8859453 - Flags: review?(michael)
Created attachment 8859454 [details] [diff] [review]
part 2.  Move the setting of mType from HTMLMenuElement::ParseAttribute to HTMLMenuElement::AfterSetAttr

This also fixes mType for cases in which the type attribute is removed.
Attachment #8859454 - Flags: review?(michael)
Created attachment 8859455 [details] [diff] [review]
part 3.  Move the setting of mType from HTMLMenuItemElement::ParseAttribute to HTMLMenuItemElement::AfterSetAttr

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+

Comment 8

a month ago
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
https://hg.mozilla.org/mozilla-central/rev/8a8d9ca30319
https://hg.mozilla.org/mozilla-central/rev/ff8904e0ad25
https://hg.mozilla.org/mozilla-central/rev/b4d447189d4d
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.