Closed Bug 606528 Opened 14 years ago Closed 6 years ago

The type attribute of inputs shouldn't be mapped

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

Don't map the type attribute for inputs
(deleted), patch
emilio
: review+
Details | Diff | Splinter Review
Same thing as diff -w for ease of review
(deleted), patch
Details | Diff | Splinter Review 
We don't need to make this a mapped attribute; it just affects what the mapping function looks like.  And changes to this attribute cause a reframe, so it's ok to just switch the mapping function when it changes.

I wish we could also not map the various attrs image inputs need unless type=image, but we don't have a good way to change from mapped to unmapped for attrs....
Attached patch Don't map the type attribute for inputs. (obsolete) (deleted) — — Splinter Review 

  

  



    
  

        Attachment #485336 -
        Flags: review?(dbaron)

    
  
Whiteboard: [need review]

    
  
Blocks: 473178

    
    

    
  
Comment on attachment 485336 [details] [diff] [review]
Don't map the type attribute for inputs.

r=dbaron if you add a comment to GetAttributeMappingFunction that you're depending on other code causing a reframe.

  

  



        Attachment #485336 -
        Flags: review?(dbaron) → review+

    
    

    
  
Was this landed?

  

  



    
    

    
  
No, for reasons that should be obvious (not a blocker, not approved).  I have no plans to land this before we ship 2.0.

  

  


Whiteboard: [need review] → [need gk2 ship]

    
    

    
  
This patch is wrong; I should have written some tests.  In particular, the mapping function is baked into our mapped attrs, so we have to rebuild those when it changes....

  

  


Whiteboard: [need gk2 ship]

    
  

        Attachment #485336 -
        Attachment is obsolete: true

    
  

        Attachment #485336 -
        Attachment is obsolete: false

        Attachment #485336 -
        Flags: review+ → review-

    
    

    
  
Still working on this?

  

  


Flags: needinfo?(bzbarsky)

    
    

    
  


  

  

  



        Attachment #8989673 -
        Flags: review?(emilio)

    
  

        Attachment #485336 -
        Attachment is obsolete: true

    
    

    
  


  

  

  



    
  
Flags: needinfo?(bzbarsky)

    
    

    
  
Comment on attachment 8989673 [details] [diff] [review]
Don't map the type attribute for inputs

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

::: dom/base/nsAttrAndChildArray.h
@@ +128,5 @@
>  
> +  // Update the rule mapping function on our mapped attributes, if we have any.
> +  // We take a nsMappedAttributeElement, not a nsMapRuleToAttributesFunc,
> +  // because the latter is defined in a header we can't include here.
> +  nsresult UpdateMappedAttrRuleMapper(nsMappedAttributeElement* aElement)

nit: Maybe worth using a reference?

::: layout/reftests/forms/input/image/type-change-from-image-1-ref.html
@@ +1,1 @@
> +<!DOCTYPE html>

These may be worth upstreaming.

  

  



        Attachment #8989673 -
        Flags: review?(emilio) → review+

    
    

    
  
> nit: Maybe worth using a reference?

Done.

> These may be worth upstreaming.

Yeah, I thought about that, but I'm not sure where I put tests-to-be-upstreamed nowadays.  Happy to move them somewhere.

This patch makes the dom/collections/namednodemap-supported-property-names.html web platform test pass, because we no longer reorder the attributes in it.  ;)

  

  



    
    

    
  
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> > nit: Maybe worth using a reference?
> 
> Done.
> 
> > These may be worth upstreaming.
> 
> Yeah, I thought about that, but I'm not sure where I put
> tests-to-be-upstreamed nowadays.  Happy to move them somewhere.

layout/reftests/w3c-css/submitted/ should work, or just landing directly in testing/web-platform/tests.

> This patch makes the
> dom/collections/namednodemap-supported-property-names.html web platform test
> pass, because we no longer reorder the attributes in it.  ;)

Nice!

  

  



    
    

    
  
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7882a06d95b5
Don't map the type attribute for inputs.  r=emilio

  

  



    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7882a06d95b5

  

  


Status: NEW → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

    
    

    
  
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11843 for changes under testing/web-platform/tests

  

  



    
    

    
  
Upstream PR merged

  

  



          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: