Closed Bug 1223561 Opened 9 years ago Closed 8 years ago

"Assertion failure: !aBuilder->IsForPainting() || !ShouldInTopLayerForFullscreen(elem)" with full-screened HTML <table>

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox53 --- fixed

People

(Reporter: jruderman, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

testcase
455 bytes, text/html
Details
stack
17.88 KB, text/plain
Details
Bug 1223561 - Support putting <table> into fullscreen.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Attached file testcase — 
Assertion failure: !aBuilder->IsForPainting() || !ShouldInTopLayerForFullscreen(elem), at layout/generic/nsViewportFrame.cpp:130
Attached file stack — 
Component: Layout → Layout: View Rendering

    
    

    
  
It would be great if someone brought this to me earilier :/
Blocks: fullscreen-api

    
  
Flags: needinfo?(xidorn+moz)

    
    

    
  
So the issue here is, when we want to put <table> into fullscreen, we are actually putting nsTableWrapperFrame into fullscreen, but the wrapper frame doesn't inherit the top layer property, so it wouldn't be positioned correctly.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8813024 [details]
Bug 1223561 - Support putting <table> into fullscreen.

https://reviewboard.mozilla.org/r/94546/#review95048

::: dom/html/test/file_fullscreen-table.html:1
(Diff revision 1)
> +<!DOCTYPE html>

I presume you've tested that this fails without the patch?

::: dom/html/test/mochitest.ini:74
(Diff revision 1)
>    file_fullscreen-plugins.html
>    file_fullscreen-rollback.html
>    file_fullscreen-scrollbar.html
>    file_fullscreen-selector.html
>    file_fullscreen-svg-element.html
> +  file_fullscreen-table.html

Please associate new support files with the test that they're a support file for, not the directory as a whole.  (Feel free to fix the old ones for this test while you're there, too.)

(Listing them as support files for all tests in the directory was just a hack to migrate to mochitest.ini from the old Makefile.in solution.)

::: layout/generic/nsPlaceholderFrame.cpp:209
(Diff revision 1)
> +  nsIFrame* parentFrame = GetParent();
> +  if ((GetStateBits() & PLACEHOLDER_FOR_TOPLAYER) &&
> +      parentFrame->GetType() == nsGkAtoms::tableWrapperFrame) {
> +    MOZ_ASSERT(mOutOfFlowFrame->GetType() == nsGkAtoms::backdropFrame,
> +               "Only placeholder of backdrop frame can be put inside "
> +               "a table wrapper frame");
> +    return parentFrame->StyleContext();
> +  }

I don't understand why you need to do make the placeholder inherit from the table wrapper frame.  Please add a comment explaining why.

::: layout/style/res/ua.css:26
(Diff revision 1)
>    box-sizing: border-box; /* XXX do we really want this? */
>  }
>  
>  *|*::-moz-table-wrapper {
>    display: inherit !important; /* table or inline-table */
> +  -moz-top-layer: inherit !important;

I think this is correct since the -moz-top-layer looks like (from reading nsCSSFrameConstructor) it will probably be ignored for the table frame.  But I'd like to confirm that agrees with your understanding.

(I also suspect there should perhaps be a comment here that says the inherited properties need to be safe to have on both the table and the table wrapper, generally because code ignores them for the table.)

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

    
    

    
  
Comment on attachment 8813024 [details]
Bug 1223561 - Support putting <table> into fullscreen.

https://reviewboard.mozilla.org/r/94546/#review95048

> I presume you've tested that this fails without the patch?

I didn't actually run the test itself without the patch... but I manually tested the prototype of this test, which shows in debug build, it would crash because of the assertion, and in release build, the table would not be put into fullscreen.

> I don't understand why you need to do make the placeholder inherit from the table wrapper frame.  Please add a comment explaining why.

It is not necessary to make the backdrop placeholder inherit from the table wrapper frame. This is just a consequence of the backdrop construction code (see nsFrameConstructorState::ConstructBackdropFrameFor).

I can make it inherit from the table frame (while still be a child of the table wrapper frame), but that wouldn't make anything simpler: it would still need a special handling in this method, because CorrectStyleParentFrame below only goes upwards along the frame tree, thus it wouldn't reach the table frame from the table wrapper frame either.

Another option is to make backdrop placeholder a child of the table frame instead. However, what really is in the top layer is the table wrapper frame, which means making backdrop placeholder under the table frame would require extra special handling in both frame construction code and painting code.

So it seems to me having it inherit from table wrapper frame is the simplest approach, and I don't see any reason we shouldn't be doing so.

    
    

    
  
Comment on attachment 8813024 [details]
Bug 1223561 - Support putting <table> into fullscreen.

https://reviewboard.mozilla.org/r/94546/#review95048

> I think this is correct since the -moz-top-layer looks like (from reading nsCSSFrameConstructor) it will probably be ignored for the table frame.  But I'd like to confirm that agrees with your understanding.
> 
> (I also suspect there should perhaps be a comment here that says the inherited properties need to be safe to have on both the table and the table wrapper, generally because code ignores them for the table.)

Yes, that matches my understanding. -moz-top-layer is handled in nsFrameConstructorState::AddChild, but table frame is appended to its parent (the table wrapper frame) via SetInitialChildList directly.

    
    

    
      
  

    
    

    
  
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/535f8682016b
Support putting <table> into fullscreen. r=dbaron

    
    

    
  
Sorry had to back out for test_fullscreen-api.html timeout, https://treeherder.mozilla.org/logviewer.html#?job_id=7078671&repo=autoland#L6339
Flags: needinfo?(xidorn+moz)

    
    

    
  
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4044c905be7b
Backed out changeset 535f8682016b for test_fullscreen-api.html timeout

    
    

    
  
Hmmm, it isn't really timeout. It crashes on some Linux opt builds inside ElementRestyler::RestyleSelf...

    
    

    
      
  

    
  
Flags: needinfo?(xidorn+moz)

    
    

    
  
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a214a23b603e
Support putting <table> into fullscreen. r=dbaron

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a214a23b603e
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: Layout: View Rendering → Layout: Web Painting

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: