Closed Bug 364188 Opened 18 years ago Closed 16 years ago

param closes open div elements

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: jingoro, Assigned: martijn.martijn)

References

(
URL
)

Details

(Keywords: fixed1.9.1, testcase)

Attachments

(3 files)

testcase
219 bytes, text/html
Details
patch?
2.67 KB, patch
mrbkap
: review+
mrbkap
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
test
496 bytes, patch
mrbkap
: review+
mrbkap
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.8) Gecko/20061025 Firefox/1.5.0.8
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.8) Gecko/20061025 Firefox/1.5.0.8

A <param> element without surrounding <object> or <applet> tags will break out of any <div> container blocks.

Interstingly, a <table><tr><td> tag will prevent it from breaking out.

See the above URL for an example.

Reproducible: Always

Steps to Reproduce:
See the above URL for an example.
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → 1.8 Branch
Our Parser turns <div><param> into <div/><param> unlike IE, Safari and Opera.

I think the latter are in line with HTML 5.
Status: UNCONFIRMED → NEW
Component: Layout → HTML: Parser
Ever confirmed: true
QA Contact: layout → parser
Summary: param without object or applet containers breaks div containers → param closes open div elements
Version: 1.8 Branch → Trunk
Attached file testcase — 

    
  
Keywords: testcase

    
    

    
  
Apparently myspace profiles have this problem see the duped bug in comment 4. Requesting blocking.
Flags: blocking1.9.2?
Flags: blocking1.9.1?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch?
        — 
          — Splinter Review
      

    


  
This seems to fix it.

        Attachment #355805 -
        Flags: review?(mrbkap)

    
    

    
  
Comment on attachment 355805 [details] [diff] [review]
patch?

This doesn't seem to fix it for me... The div is still below the border

    
    

    
  
(In reply to comment #7)
> (From update of attachment 355805 [details] [diff] [review])
> This doesn't seem to fix it for me... The div is still below the border

Sorry for the bugspam, it actually works perfectly, didn't build right last time.

        Attachment #355805 -
        Flags: review?(mrbkap) → review+

    
    

    
  
Comment on attachment 355805 [details] [diff] [review]
patch?

If this mochitests well, I think we should try it.

        Attachment #355805 -
        Flags: superreview+

    
    

    
  
passes all /parser and /layout tests, adding checkin-needed keyword, Martijn if you plan on checking it in yourself just remove it.
Assignee: nobody → martijn.martijn
Keywords: checkin-needed

    
    

    
  
Not blocking on this, but we want this for the next release.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Flags: blocking1.9.1?
Flags: blocking1.9.1-

    
    

    
  
Pushed c9b30ccee0e0
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED

    
  

        Attachment #355805 -
        Flags: approval1.9.1?

    
    

    
  
Comment on attachment 355805 [details] [diff] [review]
patch?

Please renominate for approval when we have a test.

        Attachment #355805 -
        Flags: approval1.9.1?

    
    

    
  
I'm trying to make a reftest out of the testcase Matijn uploaded here, but I can't find a reftest.list under htmlparser/tests, is there a different way of running reftests for the parser?
Whiteboard: [needs test]

    
    

    
  
Yes, we don't seem to use reftests for the parser. I'm not sure what we do for automated testing of the parser. There's a few mochitests in there, maybe that's the preferred way.

    
    

    
  
We've been using mochitests (see test_html5_tree_constructions.html).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test
        — 
          — Splinter Review
      

    


  
I added this testcase to regressions.txt, I hope that's ok. I verified that it fails on 1.9.1 and passes on current trunk.

        Attachment #368802 -
        Flags: review?(mrbkap)

        Attachment #368802 -
        Flags: superreview+

        Attachment #368802 -
        Flags: review?(mrbkap)

        Attachment #368802 -
        Flags: review+
Keywords: checkin-needed
Whiteboard: [needs test] → [needs test][c-n: test]
Whiteboard: [needs test][c-n: test] → [c-n: test]

    
    

    
  
Comment on attachment 355805 [details] [diff] [review]
patch?

The test is ready to land, so re-noming.

        Attachment #355805 -
        Flags: approval1.9.1?

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/175ed62bdef0
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: [c-n: test]
Target Milestone: --- → mozilla1.9.2a1

    
    

    
  
Comment on attachment 355805 [details] [diff] [review]
patch?

a191=beltzner

        Attachment #355805 -
        Flags: approval1.9.1? → approval1.9.1+

    
    

    
  
Comment on attachment 368802 [details] [diff] [review]
test

a191=beltzner

        Attachment #368802 -
        Flags: approval1.9.1+
Keywords: checkin-needed
Whiteboard: [needs 191 landing]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: