Closed Bug 1118169 Opened 9 years ago Closed 9 years ago

Support -moz-window-dragging:drag in HTML

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(1 file, 8 obsolete files)

v6
11.23 KB, patch
Details | Diff | Splinter Review 
We want to be able to build Desktop UIs without using XUL. Bug 1118134 introduces a way to draw HTML in the titlebar. If we draw in the titlebar, the native titlebar doesn't exist anymore, and we lose the window dragging function.

In XUL, we can do:

> #foo {
>  -moz-window-dragging: drag;
> }

Then the user can drag the window from the #foo.

The magic happens here:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsBoxFrame.cpp#1337
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1237

So I believe it's just a matter of calling AdjustWindowDraggingRegion in HTML code instead of XUL code.
Component: Graphics → Layout
Attached patch WIP (obsolete) — — Splinter Review 
Just a quick hack. It appears to work.

Markus, is that the right way to do it? I had to ignore canvasFrames. It's the anonymous content frame, right? Without that, it doesn't work.

Next steps: only enable this if we are in the top level window and if it's a certified app.
Attachment #8544544 - Flags: feedback?(mstange)
We should be smarter about when to enable/disable this feature.

We should allow this feature for non-top level windows. The draggable areas will be defined in the system & browser apps. And we should make it possible for certified app to not be slowed down by this feature.

A couple of options:

1) Allow window-dragging:drag if certified app, and <html draggable>
2) Allow window-dragging:drag if certified app, and draggable permission
3) Allow window-dragging:drag if certified app, and document embed in a <iframe mozallowwindowdrag>
4) Allow window-dragging:drag if content window flagged from privileged code (contentWindow.setDraggable(true))

Fabrice, a recommendation?
Flags: needinfo?(fabrice)
The naive way I would do that:

Add a method to presContext. presContext->SetIsDraggable(bool). Set it from nsDOMWindowUtils (accessible in JS from privileged code).

What is the life time of a presContext compared to a docshell? It doesn't survive a reload, and it gets destroyed if the viewer is hidden, right?
Comment on attachment 8544544 [details] [diff] [review]
WIP

What exactly happens if you drop the canvasFrame check?

Also, if you add this call here you'll need to remove the corresponding nsBoxFrame one, otherwise AdjustWindowDraggingRegion will be called twice for XUL frames.
Attachment #8544544 - Flags: feedback?(mstange) → feedback+
(In reply to Markus Stange [:mstange] from comment #4)
> Comment on attachment 8544544 [details] [diff] [review]
> WIP
> 
> What exactly happens if you drop the canvasFrame check?

Drag doesn't work.

In my tiny test (just running shell.html without Gaia, with * {window-drag:drag}):

I was getting 4 calls to mWindowDraggingRegion.OrWith(borderBox),
followed by 4 calls to mWindowDraggingRegion.SubOut(borderBox);

The SubOut were all triggered by Canvas(html) frames.

But now I think about it, maybe these canvas elements were created by some code in shell.html.

Not sure…

Frame dump: http://pastebin.mozilla.org/8165266
nsFrame::nsFrame could call presContext->IsDraggable(), which would return docShell->isBrowserOrApp(), or something a bit more fancy (see comment 2).

I guess that would work.
Comment on attachment 8544544 [details] [diff] [review]
WIP

Oh, you need to do aBuilder->AdjustWindowDraggingRegion(child), not "this".
Attachment #8544544 - Flags: feedback+ → feedback-
(In reply to Markus Stange [:mstange] from comment #7)
> Comment on attachment 8544544 [details] [diff] [review]
> WIP
> 
> Oh, you need to do aBuilder->AdjustWindowDraggingRegion(child), not "this".

Indeed. It works better :)
Markus, do you think it's reasonable to allow drag for all the certified apps?

If not, does it make sense to follow the allowfullscreen mechanism? (see http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2534)
Flags: needinfo?(mstange)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #9)
> Markus, do you think it's reasonable to allow drag for all the certified
> apps?

I think so... but I can't really imagine the use cases. I understand that firefox.html needs it, and that the B2G emulator needs to have *some* draggable space once the titlebar is hidden. But I don't know why B2G apps would want to have parts of themselves draggable. Aren't they intended for phones? Draggability doesn't make sense in a phone context. Or are we planning to provide a solution for native-looking desktop webapps?
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #10)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #9)
> > Markus, do you think it's reasonable to allow drag for all the certified
> > apps?
> 
> I think so... but I can't really imagine the use cases. I understand that
> firefox.html needs it, and that the B2G emulator needs to have *some*
> draggable space once the titlebar is hidden. But I don't know why B2G apps
> would want to have parts of themselves draggable. Aren't they intended for
> phones? Draggability doesn't make sense in a phone context. Or are we
> planning to provide a solution for native-looking desktop webapps?

Right. Certified apps only, on desktop only.
Attached patch wip (obsolete) — — Splinter Review 
WIP. Doesn't appear to work with sub documents.
Attachment #8544544 - Attachment is obsolete: true
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #2)
> We should be smarter about when to enable/disable this feature.
> 
> We should allow this feature for non-top level windows. The draggable areas
> will be defined in the system & browser apps. And we should make it possible
> for certified app to not be slowed down by this feature.
> 
> A couple of options:
> 
> 1) Allow window-dragging:drag if certified app, and <html draggable>
> 2) Allow window-dragging:drag if certified app, and draggable permission
> 3) Allow window-dragging:drag if certified app, and document embed in a
> <iframe mozallowwindowdrag>
> 4) Allow window-dragging:drag if content window flagged from privileged code
> (contentWindow.setDraggable(true))
> 
> Fabrice, a recommendation?

I would do 4) if we're confident that's enough for our needs.
Flags: needinfo?(fabrice)
Attached patch v0.1 (obsolete) — — Splinter Review 
(Still early draft)

So I went for option 4: `docshell.windowDragAllowed = true/false`.

Set to true by default for chrome docshells.
Always false for non-desktop builds.

Appears to work well at least for B2G desktop.

But it doesn't work for sub windows (the app is in an iframe, so 2 windows).

I need some guidance here.

We can build a draggingRegion for the content window (the app). The dragging region is used in `nsLayoutUtils::PaintFrame` with `widget->UpdateWindowDraggingRegion()`. Is that supposed to also update the dragging region of the native window?
Attachment #8544971 - Attachment is obsolete: true
Attachment #8545693 - Flags: feedback?(mstange)
Comment on attachment 8545693 [details] [diff] [review]
v0.1

Nope. Actually, my patch works.

It's just that I was using the drag property on a transformed element.
Attachment #8545693 - Flags: feedback?(mstange)
Attached patch v1 (obsolete) — — Splinter Review 
https://hg.mozilla.org/try/rev/ec0f007f0640
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8546482 - Flags: review?(mstange)
OS: Mac OS X → All
Hardware: x86 → All
I don't know if a property on the docshell is the best security model here, and unfortunately I don't know who'd know the answer. Maybe we should do something similar to how allowfullscreen works? There seems to be an iframe attribute and an nsIPermissionManager-type permission for it...

About the patch: Mostly looks good, but you really don't want to add a field to nsIFrame. There are lots of nsIFrames alive at any time, so you'd waste a lot of memory. If you need to cache the flag for performance reasons, better cache it in nsPresContext. Getting the prescontext from the frame should be fast.
Comment on attachment 8546482 [details] [diff] [review]
v1

bz might know.

Boris, we want to allow `-moz-window-dragging:drag` for HTML content, for content docshell. We are looking for the right way to dynamically enable this property. In attachment 8546482 [details] [diff] [review], I introduce a flag in the docshell. Markus is suggesting to do something similar to how allowfullscreen works. What would you recommend?
Attachment #8546482 - Flags: review?(mstange) → feedback?(bzbarsky)
So the earlier comments talk about this state per-window.  Do you want it per-(inner)-window ore per-docshell?  What should happen to the ability to trigger moz-window-dragging when the docshell is navigated, for example?
Flags: needinfo?(paul)
(In reply to Boris Zbarsky [:bz] from comment #20)
> So the earlier comments talk about this state per-window.  Do you want it
> per-(inner)-window ore per-docshell?  What should happen to the ability to
> trigger moz-window-dragging when the docshell is navigated, for example?

Per docshell: Enable window dragging. Navigate. Window dragging still works.
Flags: needinfo?(paul)
In that case, the docshell attribute makes sense.  The allowfullscreen stuff iirc wants to decide based on site and so forth, which is why it has a permission.
Attached patch v2 (obsolete) — — Splinter Review 
Markus, is that better? I kept the docshell property as per comment 22.

Also, it doesn't appear to work with `position:absolute` elements. Any idea why?
Attachment #8545693 - Attachment is obsolete: true
Attachment #8546482 - Attachment is obsolete: true
Attachment #8546482 - Flags: feedback?(bzbarsky)
Attachment #8549406 - Flags: feedback?(mstange)
> Also, it doesn't appear to work with `position:absolute` elements. Any idea
> why?

It seems to work for me. I applied your patch, removed the "if (PresContext()->GetWindowDraggingAllowed())" check for testing purposes, added "-moz-window-dragging: drag; position: absolute; top: 0;" to the "main h1" rule on https://www.mozilla.org/en-US/firefox/nightly/firstrun/?oldversion=35.0 using the inspector, and was successfully able to drag the "Thank you for using Firefox Nightly" string. Are you sure that there's no transform involved in the thing you're testing on?
Comment on attachment 8549406 [details] [diff] [review]
v2

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

This looks good, but I'm going to go back on my previous suggestion because I now have something better in mind.

Instead of caching mDraggingAllowed in the PresContext, we can cache it in the DisplayListBuilder, and only update it in Enter/LeavePresShell. There you could get the value from the PresContext and have that directly ask the DocShell every time, or you could access the DocShell directly in Enter/LeavePresShell and leave the PresContext completely out of the equation. Not sure what works better, you'll need to try it out.
Attachment #8549406 - Flags: feedback?(mstange) → feedback+
Attached patch v3 (obsolete) — — Splinter Review 

        Attachment #8549406 -
        Attachment is obsolete: true

        Attachment #8555659 -
        Flags: review?(mstange)

    
    

    
  
Comment on attachment 8555659 [details] [diff] [review]
v3

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

r+ with the ::LeavePresShell issue fixed. And you'll need DOM peer review on the docshell change.

::: docshell/base/nsDocShell.cpp
@@ +3032,5 @@
> +  *aValue = false;
> +#else
> +  nsCOMPtr<nsIDocShellTreeItem> parent =
> +    do_QueryInterface(GetAsSupports(mParent));
> +  if (mItemType == typeChrome && !parent) {

Does && !GetParentDocshell() work?

::: layout/base/nsDisplayList.cpp
@@ +1006,5 @@
>      mFramesMarkedForDisplay.AppendElement(state->mCaretFrame);
>      MarkFrameForDisplay(state->mCaretFrame, nullptr);
>    }
> +
> +  aReferenceFrame->PresContext()->GetDocShell()->GetWindowDraggingAllowed(&mWindowDraggingAllowed);

nsPresContext* pc = aReferenceFrame->PresContext();
  mWindowDraggingAllowed = pc->GetDocShell()->GetWindowDraggingAllowed();

for lower line length and better readability

@@ +1017,5 @@
>        aReferenceFrame->PresContext()->PresShell(),
>        "Presshell mismatch");
>    ResetMarkedFramesForDisplayList();
>    mPresShellStates.SetLength(mPresShellStates.Length() - 1);
> +  mWindowDraggingAllowed = false;

Why false? I think you need to get the correct new value here for the parent presshell.

  nsPresContext* pc = CurrentPresContext();
  mWindowDraggingAllowed = pc->GetDocShell()->GetWindowDraggingAllowed();

        Attachment #8555659 -
        Flags: review?(mstange) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v4 (obsolete)
        — 
          — Splinter Review
      

    


  
Markus, re-asking you to review this patch as I'm not confident with the ::LeavePresShell() change. For a reason I ignore, calling `pc->GetDocShell()->GetWindowDraggingAllowed` after `ResetMarkedFramesForDisplayList()/SetLength()` crashes Firefox. Moving it above appears to work. But I'm really not familiar with this code.

        Attachment #8555659 -
        Attachment is obsolete: true

        Attachment #8555720 -
        Flags: review?(mstange)

        Attachment #8555720 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8555720 [details] [diff] [review]
v4

Why do we really want the gonk ifdef here?  In which cases do we expect it to affect the behavior?  If we do need that, why shouldn't the setter throw on gonk?
Flags: needinfo?(paul)

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8555720 [details] [diff] [review]
> v4
> 
> Why do we really want the gonk ifdef here?  In which cases do we expect it
> to affect the behavior?

Isn't the top level window in B2G a chrome docshell?
If so, we don't want to allow window dragging, and I don't see any other way to do that.

> If we do need that, why shouldn't the setter throw on gonk?

Right.
Flags: needinfo?(paul) → needinfo?(bzbarsky)

    
    

    
  
Window dragging is implemented at the widget level. The widget knows about the draggable region because layout calls nsIWidget::SetWindowDraggableRegion. nsChildView (which is Mac-only) is the only nsIWidget implementation that actually does something in response to nsIWidget::SetWindowDraggableRegion. On Gonk we don't have nsChildView so allowing window dragging won't have any effect. So I think we should allow it everywhere.

    
    

    
  
(In reply to Paul Rouget [:paul] from comment #28)
> Markus, re-asking you to review this patch as I'm not confident with the
> ::LeavePresShell() change. For a reason I ignore, calling
> `pc->GetDocShell()->GetWindowDraggingAllowed` after
> `ResetMarkedFramesForDisplayList()/SetLength()` crashes Firefox.

Oh, right. If you exit the outermost presshell, you're probably not allowed to call CurrentPresContext(), since mPresShellStates will be empty. Is the crash fixed if you guard the two lines I suggested with !mPresShellStates.IsEmpty()?

> Moving it above appears to work.

Well, that would give us the value for the presshell we just left. But that's not the one we want.

    
    

    
  
I think comment 31 answers the question you were asking?
Flags: needinfo?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v5 (obsolete)
        — 
          — Splinter Review
      

    


  
Markus, the isEmpty() check works.

        Attachment #8555720 -
        Attachment is obsolete: true

        Attachment #8555720 -
        Flags: review?(mstange)

        Attachment #8555720 -
        Flags: review?(bzbarsky)

        Attachment #8556276 -
        Flags: review?(mstange)

        Attachment #8556276 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8556276 [details] [diff] [review]
v5

r=me on the docshell bits

        Attachment #8556276 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Comment on attachment 8556276 [details] [diff] [review]
v5

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

::: docshell/base/nsDocShell.cpp
@@ +3020,5 @@
> +  nsRefPtr<nsDocShell> parent = GetParentDocshell();
> +  if (!aValue && mItemType == typeChrome && !parent) {
> +    // Window dragging is always allowed for top level
> +    // chrome docshells.
> +    return NS_ERROR_FAILURE;

Hmm, ok. So you changed this from determining the default value of docShell.windowDraggingAllowed to determining the only allowed value of the property. Both approaches have merit, I guess.

        Attachment #8556276 -
        Flags: review?(mstange) → review+

    
    

    
  
Looking at try, it hits this assert:

> Assertion failure: referenceFrame == mReferenceFrame (referenceFrameToRootReferenceFrame needs to be adjusted), at /builds/slave/try-osx64-d-000000000000000000/build/src/layout/base/nsDisplayList.cpp:1271

Markus, any idea what's wrong?
Flags: needinfo?(mstange)

    
    

    
  
Not really, would have to debug.
Flags: needinfo?(mstange)

    
    

    
  
The assertion comes from bug 1104036 you recently fixed.

Why is the assertion useful? Asking because with an opt build, everything appears to work fine.

I'm wondering when we hit this assertion. We removed the 'IsInSubdocument()' check, but this is done by mWindowDraggingAllowed, which is false for sub documents. So I don't understand why we would hit this assertion with Firefox Desktop.
Flags: needinfo?(mstange)

    
    

    
  
(In reply to Paul Rouget [:paul] from comment #40)
> Why is the assertion useful? Asking because with an opt build, everything
> appears to work fine.

It's useful because if we're able to hit it, it's probably possible to create a testcase in which the draggable region is in the wrong place.

> I'm wondering when we hit this assertion. We removed the 'IsInSubdocument()'
> check, but this is done by mWindowDraggingAllowed, which is false for sub
> documents. So I don't understand why we would hit this assertion with
> Firefox Desktop.

I don't know either. I'll try to reproduce it locally.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v6
        — 
          — Splinter Review
      

    


  
I found the bug. We were calling AdjustWindowDraggingRegion from the wrong place, such that IsInTransform() wasn't returning the right result.

        Attachment #8556276 -
        Attachment is obsolete: true
Flags: needinfo?(mstange)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6f69ad74baa5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: