Closed Bug 178513 Opened 22 years ago Closed 17 years ago

Make text, images translucent while dragging instead of outline

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, enhancement, P3)

PowerPC
macOS
enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: plastercast, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 9 obsolete files)

When dragging an object, such as some text, instead of an outline, Chimera
should show a transparent version of the text (a la IE).  The same should be
true of images.
Confirming as an RFE. Another aspect of Chimera's use of the Carbon drag manager
(bug 152580)?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Dragging an object should result in transparent object (a la IE) → Make text, images translucent while dragging instead of outline
This bug is specific to camino but it is dependent on 191822.
Depends on: 191822
Status of this? Bug 191822 is apparently done with Win32 work (in 2003). Is this
something that's fairly easy to do as it's a standard Mac feature? Or is it
somehow dependent on Quartz?

Also, if bug 191822 has all other work (except Mac) finished, maybe we should
close that one and make this a general Core Mac bug.
Priority: -- → P3
Target Milestone: --- → Camino1.2
This is certainly something we need to see on Mac OS X as it is an eye sore to have that rectangular frame.
QA Contact: chrispetersen → drag.drop
Target Milestone: Camino1.2 → Camino2.0
This seems to be partially supported in nsDragService already:

http://lxr.mozilla.org/mozilla/source/widget/src/mac/nsDragService.cpp#132

Pink said in the comment that it "doesn't work too well", so I'm guessing we've #ifdef'd it out somewhere.

CCing him to see if he has further commentary.

cl
See also bug 307857, which wants Safari-style info bezels when dragging link-type objects.
*** Bug 358655 has been marked as a duplicate of this bug. ***
This is core now. Josh, feel free to dup if you already have a bug tracking this from your DnD work.
Assignee: sfraser_bugs → nobody
Component: Drag & Drop → Drag and Drop
OS: Mac OS X 10.2 → Mac OS X 10.4
Product: Camino → Core
QA Contact: drag.drop
Target Milestone: Camino2.0 → ---
Version: unspecified → Trunk
*** Bug 191822 has been marked as a duplicate of this bug. ***
Attached patch content related (obsolete) — Splinter Review
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attached patch Graphics/widget related (obsolete) — Splinter Review
Implements generic and gtk2 support for drag feedback
Attachment #246111 - Flags: superreview?(roc)
Attachment #246111 - Flags: review?(vladimir)
Split into multiple patches for easier review by different folks.
Attachment #246112 - Flags: superreview?(roc)
Attachment #246112 - Flags: review?(joshmoz)
With a cocoa build (non-cairo), I get this error when compiling nsBaseDragService.o:

In file included from .../mozilla/widget/src/xpwidgets/nsBaseDragService.cpp:60:
../../../dist/include/layout/nsFrameSelection.h:670:71: error: invalid suffix
"a2" on integer constant
Attached patch Fix constant (obsolete) — Splinter Review
Bah, I made the patch before saving a file. This should be the right patch.
Attachment #246110 - Attachment is obsolete: true
Comment on attachment 246112 [details] [diff] [review]
Mac Cocoa support

Thanks, that builds for me.

The main problem I have with the behavior of this patch is that when selecting text and then dragging it, the image that gets dragged often includes text that was not selected. This is quite misleading about what is actually getting dragged, people may think they are copying more text than they really are which is a mild form of data loss.

Whatever the scheme for creating an image from text, it should never make it look as though more text is being dragged than actually is.

Safari doesn't allow text dragging, so it is a non-issue for them it seems. Maybe there is a way to do it and I just don't know how though.

Test case: select half of the first line of a wrapped paragraph of text and half of the second line of that paragraph. Drag the selection. The entire first and second lines will be in the drag image.
Attachment #246112 - Flags: review?(joshmoz) → review-
Josh, are you using a cairo build? My cairo build does that as well, and I think it's a bug in the clip region handling unrelated to this patch. My non-cairo build clips properly.
I am using a cairo build. I think we should make fixing that a priority if this is to land, but so long as that is a bug somewhere else and not in your code I'm fine with continuing to review.
(In reply to comment #16)

> Safari doesn't allow text dragging, so it is a non-issue for them it seems.
> Maybe there is a way to do it and I just don't know how though.

It does; you just have to hold the button for a bit second before dragging (like other cocoa stuff).
Blocks: 356295
Comment on attachment 246112 [details] [diff] [review]
Mac Cocoa support

r+ since it would be good to have this in earlier, we need to make fixing the clipping bug on cocoa-cairo a priority though
Attachment #246112 - Flags: superreview?(roc) → superreview+
Attachment #246112 - Flags: superreview?(roc)
Attachment #246112 - Flags: superreview+
Attachment #246112 - Flags: review-
Attachment #246112 - Flags: review+
Comment on attachment 246112 [details] [diff] [review]
Mac Cocoa support

+  if (surface->GetType() == gfxASurface::SurfaceTypeImage) {
+    imgSurface = NS_STATIC_CAST(gfxImageSurface*,
+                                NS_STATIC_CAST(gfxASurface*, surface));
+  }
+  else {

I think this isn't worth it. This path is not performance critical. Just always create a new temporary image. Then you can also skip the "if (imgSurface->Format() != gfxASurface::ImageFormatARGB32)" check.

Is it really worth handling the non-cairo case at all? I think it would be clearer if we just didn't bother.
(In reply to comment #21)

> I think this isn't worth it. This path is not performance critical. Just always
> create a new temporary image. Then you can also skip the "if
> (imgSurface->Format() != gfxASurface::ImageFormatARGB32)" check.
> 

OK.

> Is it really worth handling the non-cairo case at all? I think it would be
> clearer if we just didn't bother.
> 

We don't need the non-cairo parts any more. I wrote that code before cairo was enabled on mac.

OK, let's get rid of the non-cairo code. That will let you simplify some of the shared code.
(In reply to comment #23)
> OK, let's get rid of the non-cairo code. That will let you simplify some of the
> shared code.
> 

Do I still need the ifdef cairos, for those non-primary platforms?
I think we haven't yet completely disabled non-cairo builds, but the day is coming when we should. Vlad, I think we should have a flag day after which non-cairo will not build at all.
We have remove non-cairo gfx support from cocoa widgets. No need for that or any cairo ifdefs in cocoa widget code any more.
The only other change I made was to fix a transparency issue so that images appear 80% opaque
Attachment #246111 - Attachment is obsolete: true
Attachment #249198 - Flags: superreview?(roc)
Attachment #249198 - Flags: review?(vladimir)
Attachment #246111 - Flags: superreview?(roc)
Attachment #246111 - Flags: review?(vladimir)
Comment on attachment 249198 [details] [diff] [review]
remove non-cairo codepaths

Looks fine to me, at least the thebes bits, though nothing in the other portions stood out.

You may be able to optimize the mac portion to avoid going through a raw bitmap image and manually doing bit twiddling, but I couldn't figure out how to get a NSImageRep from a CGImageRef (which I can give you from the cairo Quartz surface).  Not important enough to worry about.
Attachment #249198 - Flags: review?(vladimir) → review+
+    size.height = dragRect.height;
+    image = [[NSImage alloc] initWithSize:size];
+    [image lockFocus];
+    [[NSColor grayColor] set];
+    NSBezierPath* path = [NSBezierPath bezierPath];
+    [path setLineWidth:2.0];
+    [path moveToPoint:NSMakePoint(0, 0)];
+    [path lineToPoint:NSMakePoint(0, size.height)];

Is the stroke centered on the line? If so, are we throwing away the outside half of the stroke here?

+    point.y = NSMaxY([[[NSScreen screens] objectAtIndex:0] frame]) - (dragRect.y + dragRect.height);

dragRect.YMost()

+    nsRect rectAsTwips(*aSurfaceRect);
+    rectAsTwips.ScaleRoundOut(aPresContext->PixelsToTwips());
+
+    nsRect clip(0, 0, rectAsTwips.width, rectAsTwips.height);

Isn't rectAsTwips just the same as surfaceArea?

+
+  // true if mImage should be used to set a drag image
+  PRBool mHasImage;

PRPackedBool and rearrange things so it's next to the other PRPackedBools?

In DrawDragImage, I'd like you to factor out the code that computes the rectangle to render in window (pixel) coordinates into its own function (basically the stuff under "if (dragFrame)"). And I think you should be able to do it all with frames, not views. Outside that helper function, you can then convert to screen coordinates with the help of presShell->GetRootFrame()->GetScreenRectExternal().

The image-drawing code should be split out into its own function too.

+    presShell->RenderOffscreen(rect, PR_FALSE, PR_FALSE,
+                               NS_RGB(255, 255, 255), rc, nsnull);

Why are you rendering with a white background? Why not render with a transparent background?

Since you seem to want to be clipped by the root scrollframe anyway, and pass in your own rendering context, maybe you should just be call nsIPresShell::Paint instead of RenderOffscreen. You would need to add a parameter to it to control background color painting, if you decide you want a transparent background.

Seems like this is going to be suboptimal in general when the user is dragging content that is partially covered by other content. It would be nice to just render the content subtree that the drag service is asking for. Hmm. How does this work when the user wants to drag a range, such as a range of text? Do you really want to render an nsISelection?
(In reply to comment #29)
> +    size.height = dragRect.height;
> +    image = [[NSImage alloc] initWithSize:size];
> +    [image lockFocus];
> +    [[NSColor grayColor] set];
> +    NSBezierPath* path = [NSBezierPath bezierPath];
> +    [path setLineWidth:2.0];
> +    [path moveToPoint:NSMakePoint(0, 0)];
> +    [path lineToPoint:NSMakePoint(0, size.height)];
> 
> Is the stroke centered on the line? If so, are we throwing away the outside
> half of the stroke here?

Josh wrote this part, I just moved it, so he may be able to answer.
 
> 
> Why are you rendering with a white background? Why not render with a
> transparent background?

I recall it not blending properly, but I could try again.

> 
> Since you seem to want to be clipped by the root scrollframe anyway, and pass
> in your own rendering context, maybe you should just be call
> nsIPresShell::Paint instead of RenderOffscreen. You would need to add a
> parameter to it to control background color painting, if you decide you want a
> transparent background.

Assuming you mean nsIViewObserver::Paint, that seems to work just as well. Changing the background colour doesn't have any effect that I can tell though.

> 
> Seems like this is going to be suboptimal in general when the user is dragging
> content that is partially covered by other content. It would be nice to just
> render the content subtree that the drag service is asking for.

Indeed.

> How does this work when the user wants to drag a range, such as a
> range of text? Do you really want to render an nsISelection?

An image of the selection is created and dragged. The other patch in this bug (https://bugzilla.mozilla.org/attachment.cgi?id=246179) implements that part. Feel free to review it if you can, otherwise, I'm not sure who can.

ah OK. So in that case the DOM node is the document root and we're just rendering some region of it. Well, that'll do for now. I'll review both parts, once you figure out how to deal with my comments so far :-)
Attached patch fix issues described above (obsolete) — Splinter Review
This is the entire patch
Attachment #246179 - Attachment is obsolete: true
Attachment #254442 - Flags: superreview?(roc)
Attachment #254442 - Flags: review?(roc)
Attachment #246112 - Flags: superreview?(roc)
Attachment #249198 - Flags: superreview?(roc)
Could you use a more meaningful name like "aClosure" instead of "aRefCon"? The latter is a peculiar Mac term.

+  nsresult     IterateOverFramesUnderNode(nsPresContext* aPresContext,
+                                          nsIContentIterator *aInnerIter,
+                                          nsIContent *aContent,
+                                          nsIDOMRange *aRange,
+                                          FrameIterationCallback aCallback,
+                                          void* aRefCon);
+  nsresult     IterateOverFramesInRange(nsPresContext* aPresContext,
+                                        nsIDOMRange *aRange,
+                                        FrameIterationCallback aCallback,
+                                        void* aRefCon);

Document these

+ PRBool inOnRangeBoundary

That is a strange name.

+  nsRect frameRect = aFrame->GetRect();
+  frameRect.x = frameRect.y = 0;

I prefer "nsRect frameRect(nsPoint(0,0), aFrame->GetSize());"

I think this range-to-region calculation should be in nsIRange. Someday I'd like to add getClientRects and getBoundingClientRect DOM methods to nsIDOMNSRange and they'll probably share some of this code. You need to document exactly what region is returned.

+    if (frameNode == rangeStart && frameNode == rangeEnd)
+    {

I think this can be simplified so you can handle frameNode == rangeStart and frameNode == rangeEnd with one block of code each.

I'd like you to remove that view code that's accumulating clipping. You could do the same thing using frames by QIing to nsIScrollableFrame and getting its scroll port rect and clipping to that. But maybe it would be better to have a new nsIFrame method, GetClipRectForChild(). Then we can handle other sources of clipping as well.

+    if (frameType == nsGkAtoms::textFrame    ||
+        frameType == nsGkAtoms::letterFrame  ||
+        frameType == nsGkAtoms::imageFrame)    // XXX others?

What about frames with borders, or background images, or other replaced elements such as canvas, or ...? I'd like to see this check separated out into its own function, perhaps in nsLayoutUtils? Maybe it should be "all frames except for ..." instead.

+nsTypedSelection::IterateOverFramesInRange(nsPresContext* aPresContext,

Isn't the outer iterator traversing all nodes under the range? Why do we need an inner content iterator? Sorry, I'm not familiar with content iterators.

If the user selects a lot of content including a very large image, we don't scale it down. If the user selects just the image, we do scale it down. Does this make sense? Should we perhaps scale down overlarge selections in general?

> I'd like you to remove that view code that's accumulating clipping. You could
> do the same thing using frames by QIing to nsIScrollableFrame and getting its
> scroll port rect and clipping to that. But maybe it would be better to have a
> new nsIFrame method, GetClipRectForChild(). Then we can handle other sources of
> clipping as well.

What do you mean here? Do you mean get the bounds from nsIScrollableView? Don't we still need to iterate and clip the ranges to each scrollable view?

> 
> +nsTypedSelection::IterateOverFramesInRange(nsPresContext* aPresContext,
> 
> Isn't the outer iterator traversing all nodes under the range? Why do we need
> an inner content iterator? Sorry, I'm not familiar with content iterators.

Don't know. I think it's to match the algorithm used by nsTypedSelection::selectFrames. I just adapted this code from the patch in bug 196967. Maybe as the original author, Simon would know.

> 
> If the user selects a lot of content including a very large image, we don't
> scale it down. If the user selects just the image, we do scale it down. Does
> this make sense? Should we perhaps scale down overlarge selections in general?
> 

It wouldn't be too hard to scale down. I mainly attempted to emulate the Safari behaviour for this, which scales large images, but doesn't scale selections.
(In reply to comment #34)
> > I'd like you to remove that view code that's accumulating clipping. You could
> > do the same thing using frames by QIing to nsIScrollableFrame and getting its
> > scroll port rect and clipping to that. But maybe it would be better to have a
> > new nsIFrame method, GetClipRectForChild(). Then we can handle other sources of
> > clipping as well.
> 
> What do you mean here? Do you mean get the bounds from nsIScrollableView? Don't
> we still need to iterate and clip the ranges to each scrollable view?

Yes, I just want you to iterate over frames, not views. I guess for now you will have to get the bounds of the scrollable view to get the scroll port to clip to (make sure you only clip stuff coming from GetScrolledFrame() or frames below it, don't clip the scrollbars to the scrollport...)
I wrote that a long time ago. Memory hazy.
Neil, on the assumption that the outer content iterator visits every node in the range, including descendant nodes, I think you should just union together the frames for all those nodes (along with all next-in-flows, with the extra modification to handle text nodes that are the start or end node for the range.
From testing, the outer iterator iterates over the subtree of elements, and the inner iterator iterates over the text nodes within those elements.
Why's that? They're both initialized and used the same way, as far as I can tell. Is there no documentation on these iterators?
Attached patch changes (obsolete) — Splinter Review
This patch uses only one content iterator which seems to include all the desired nodes. The subtree iterator only includes the nodes that are completely within the range. Also, I moved this much simpler code into nsRange.cpp

I didn't change the check for the frame types. Only text and image frames are part of the selection. Canvases aren't selectable, see bug 294852.
Attachment #254442 - Attachment is obsolete: true
Attachment #255769 - Flags: superreview?(roc)
Attachment #255769 - Flags: review?(roc)
Attachment #254442 - Flags: superreview?(roc)
Attachment #254442 - Flags: review?(roc)
I think AddFrameToRegion would be simpler if you just computed the start and end DOM offsets and then converted those into horizontal offsets. So always set highlightStart to the PR_MAX(...) and then always do two GetPointFromOffset calls and then always set frameRect.x and frameRect.width. (And I think you need to consider the possibility that thanks to RTL, the returned X coordinates could be in either order.)

+  frameRect.x = nsPresContext::AppUnitsToIntCSSPixels(frameRect.x);
+  frameRect.y = nsPresContext::AppUnitsToIntCSSPixels(frameRect.y);
+  frameRect.width = nsPresContext::AppUnitsToIntCSSPixels(frameRect.width);
+  frameRect.height = nsPresContext::AppUnitsToIntCSSPixels(frameRect.height);

Here I think you should call nsRect::ScaleRoundOut. And I think you should be converting to device pixels, and document that in nsRange.h.

Don't use views to create the rendering context in GetRangeRegion. Instead, call nsIPresShell::CreateRenderingContext on the presshell's root frame.

+    PRBool atStart = (content == startContent && !content->IsNodeOfType(nsINode::eELEMENT));
+    PRBool atEnd = (content == endContent && !content->IsNodeOfType(nsINode::eELEMENT));

Why do you need the node-type checks?

How about having AddFrameToRegion do its GetPointFromOffset dance only for text frames? I'm a bit worried about it applied to non-text frames.

+          frameType == nsGkAtoms::letterFrame ||

is letterFrame necessary? It should contain a textframe child that should be enough for us.

nsRange::GetRangeRegion should be renamed to AddRangeToRegion or something like that.

+  *aSelectionRegion = theRegion;
+  NS_ADDREF(*aSelectionRegion);

theRegion.swap(aSelectionRegion);

+      destRect.height = NSToIntFloor(destRect.height * MAX_IMAGE_SIZE / (float)destRect.width);

+      destRect.width = NSToIntFloor(destRect.width * MAX_IMAGE_SIZE / (float)destRect.height);

Cast to float before doing the multiply to avoid integer overflow.

I think you should define "float scale = destRect.width/srcRect.width;" and use it in various places...

DrawDragImageForImage is not a nice name. How about making this just DrawDragForImage and making the other one DrawDrag?
Attached patch fix up issues (obsolete) — Splinter Review
Fix review comments.
Also:
 - letter frames need to be handled specially to get the first child
 - there's still an issue with rtl, the first frame returns 0 for GetPointFromOffset for both endpoints.
Attachment #255769 - Attachment is obsolete: true
Attachment #256498 - Flags: superreview?(roc)
Attachment #256498 - Flags: review?(roc)
Attachment #255769 - Flags: superreview?(roc)
Attachment #255769 - Flags: review?(roc)
+    if (aAtStart && aAtEnd && hilightStart >= hilightEnd)
+      return NS_OK;

You don't need to check aAtStart && aAtEnd here.

The issues with letter frames are actually more general. This doesn't handle generated content, nor does it handle XBL content. We really need to traverse the frame tree, not the content tree. So how about this: keep a hashtable of nsIFrame* pointers that we've visited. For every content element returned by the content iterator, traverse the entire frame tree under its primary frame (following all child lists using GetAdditionalChildListName) and using the hashtable to prevent visiting the same frame more than once.

Style nit: use float() instead of (float) for casts.

I'm still a bit concerned that you're only looking at text and image frames. For example if the range contains an empty block that has a border, shouldn't we add that border box to the region?

What is this remaining issue with RTL? GetPointFromOffset should be returning the frame's width for offset 0, for RTL text.
(In reply to comment #43)
> +    if (aAtStart && aAtEnd && hilightStart >= hilightEnd)
> +      return NS_OK;
> 
> You don't need to check aAtStart && aAtEnd here.

I do if the start node isn't the same as the end node, where the offset into start is greater than the offset into the end node.

> 
> The issues with letter frames are actually more general. This doesn't handle
> generated content, nor does it handle XBL content.

Well, the selection doesn't handle generated content either (bug 12460), and selecting anonymous content isn't handled very well, so I don't think this is something that needs dealing with in this bug.

> 
> I'm still a bit concerned that you're only looking at text and image frames.
> For example if the range contains an empty block that has a border, shouldn't
> we add that border box to the region?
> 

Perhaps, although as this code was intended for getting the selection region, and non-text or non-images aren't selectable, so I don't think it is necessary for this bug.


> What is this remaining issue with RTL? GetPointFromOffset should be returning
> the frame's width for offset 0, for RTL text.
> 

In this example:

  <p style="direction: rtl;">
    This is some <strong>strong, <em>emphasized</em></strong> text.
  </p>

The first frame's GetSize returns (0, 0) yet has the content 'This is some'.
(In reply to comment #44)
> (In reply to comment #43)
> > +    if (aAtStart && aAtEnd && hilightStart >= hilightEnd)
> > +      return NS_OK;
> > 
> > You don't need to check aAtStart && aAtEnd here.
> 
> I do if the start node isn't the same as the end node, where the offset into
> start is greater than the offset into the end node.

But hilightStart should be less than hilightEnd in that case, right?

> > The issues with letter frames are actually more general. This doesn't handle
> > generated content, nor does it handle XBL content.
> 
> Well, the selection doesn't handle generated content either (bug 12460), and
> selecting anonymous content isn't handled very well, so I don't think this is
> something that needs dealing with in this bug.

OK.
 
> > I'm still a bit concerned that you're only looking at text and image frames.
> > For example if the range contains an empty block that has a border, shouldn't
> > we add that border box to the region?
> 
> Perhaps, although as this code was intended for getting the selection region,
> and non-text or non-images aren't selectable, so I don't think it is necessary
> for this bug.

Sure they're selectable. You can select the block, cut it, and paste it somewhere else in an HTML editor. Or even drag and drop it somewhere else... We just don't render the selection.

BTW have you tested this code drag-and-dropping inside a textarea? Does it actually work or does it grab the entire textarea?

BTW I think you can simplify your range code by QIing nsIDOMRange to nsIRange, which has deCOM methods for getting the start and end offsets and content as nsIContent*.

> > What is this remaining issue with RTL? GetPointFromOffset should be returning
> > the frame's width for offset 0, for RTL text.
> 
> In this example:
> 
>   <p style="direction: rtl;">
>     This is some <strong>strong, <em>emphasized</em></strong> text.
>   </p>
> 
> The first frame's GetSize returns (0, 0) yet has the content 'This is some'.

Actually bidi resolution probably split the frame for the first text node into two frames, one a string of right-to-left spaces that were entirely collapsed because they're at the start of the line, followed by a left-to-right text frame mapping "This is some ". The latter frame would be reachable from the former frame by calling GetNextContinuation() (but not GetNextInFlow() --- this is one situation where not all continuations are next-in-flows). So you can probably just call GetNextContinuation() instead of GetNextInFlow() and everything should be OK.
(In reply to comment #45)
> But hilightStart should be less than hilightEnd in that case, right?
> 

You're right. I was confused by an earlier version of that code.

> 
> Sure they're selectable. You can select the block, cut it, and paste it
> somewhere else in an HTML editor. Or even drag and drop it somewhere else... We
> just don't render the selection.

For drag feedback, I want to have it display only what appears visibly to be selected. Later, a flag could be added when GetClientRects is implemented for ranges which skips the textnode check. I'll add a comment to this effect.

> 
> BTW have you tested this code drag-and-dropping inside a textarea? Does it
> actually work or does it grab the entire textarea?
> 

Yes, the code added in PlaintextDataTransfer::DoDrag gets the textarea's selection.
> For drag feedback, I want to have it display only what appears visibly to be
> selected.

Why? I think our failure to display, say, empty blocks as selected is just that, a failure. I don't see why we need to replicate it here.
Why would the user want to select and/or drag empty boxes?

Maybe there's some confusion here. The region needed for the drag image for selections should only cover the part of page that has actual content, that is, the text (and images, and possibly other inline elements). The area of the surrounding blocks should not be included, otherwise, the image would include extra blank areas covering the block's area. Ideally, the backgrounds of all elements would also be ignored when drawn - this is the Safari behaviour. I don't see why empty blocks should be treated any differently.

(In reply to comment #48)
> Why would the user want to select and/or drag empty boxes?

Because it might have a border or a background-image or some other useful styling, and they might want to move it or copy it.

> Maybe there's some confusion here. The region needed for the drag image for
> selections should only cover the part of page that has actual content, that is,
> the text (and images, and possibly other inline elements). The area of the
> surrounding blocks should not be included, otherwise, the image would include
> extra blank areas covering the block's area. Ideally, the backgrounds of all
> elements would also be ignored when drawn - this is the Safari behaviour. I
> don't see why empty blocks should be treated any differently.

When an endpoint of the selection is inside a block, I can see that we don't want borders or backgrounds from those block container(s) to be part of the region, or part of the drawing.

When the selection fully contains blocks that have borders, backgrounds etc, I don't see why we should exclude those.
I think the drag image should contain exactly the parts of the page which are covered by the selection highlight. This is what Mac OS X does, and it makes the most sense. You're dragging what's selected. The drag should act like a copy and paste.
The problem is that some of the stuff we would copy and paste isn't highlighted.
I'm not arguing about what should or should not be part of a selection. This bug is about drag feedback. We don't drag or copy borders or backgrounds around, nor do we drag any styling around. Only the html source gets dragged, and in almost all cases, this gets converted into plain text. Maybe one day we will support dragging the appearance of an element, but this bug isn't implementing that.
(In reply to comment #52)
> I'm not arguing about what should or should not be part of a selection. This
> bug is about drag feedback. We don't drag or copy borders or backgrounds
> around, 

We sure do. Try this in an HTML editor, e.g., GMail's HTML email composer:

1. Select some text
2. Give it a background color
3. Drag it within the editor

Result: the background color moves with the text, as you'd expect.

Then try this:

1. Create an HTML document with markup like this:
<p>Hello
<div style="border:2px solid black;">Hello</div>
<p>Hello
2. Select starting in the first <p> and ending in the last <p>
3. Drag the text into an HTML editor

Result: the DIV with border is dragged into the editor, as you'd expect.

I don't mind too much if we say that we don't include this stuff in the drag region because it's too much to implement right now, or something, but I don't agree that we should say it's "by design" because borders and backgrounds "aren't dragged around". They clearly are.

(Because of plaintext editor targets and different styling in the target, it's true that the appearance of the text may change when it lands in the target. But of course it's impossible to draw the text as it will appear in the target from the beginning of the drag, because that depends on the exact target and we don't know what the target will be. I think it makes most sense to just render it as it appears in the source instead, and that also happens to be easiest to implement. We could make it so that when you drag over a target we change the image to show how the text will look in the target, but that could be confusing and would be a real pain to implement.)
If you mean we drag inline style attributes around, then yes. We still don't drag what the borders look like when drawn on the screen though.

If you have a better algorithm for determining the drag region instead of just including the text frames, feel free to specify it. I don't have a problem implementing something more precise.
(In reply to comment #54)
> We still don't
> drag what the borders look like when drawn on the screen though.

What do you mean? That depending on the stylesheets in the target the borders may be drawn differently in the target when the text is dropped? Sure.

> If you have a better algorithm for determining the drag region instead of just
> including the text frames, feel free to specify it. I don't have a problem
> implementing something more precise.

How about "add in the regions for partially selected text frames, as you're doing now, and also add in the overflow areas of the frames for elements completely enclosed by the range"? You may be able to avoid some frame tree traversals by taking advantage of the fact that the overflow area for a frame includes the overflow areas of all descendant frames.
No, because then(In reply to comment #55)
> How about "add in the regions for partially selected text frames, as you're
> doing now, and also add in the overflow areas of the frames for elements
> completely enclosed by the range"?

If there was a way to render without the solid backgrounds drawing though, that would be ok. Otherwise, you'd get huge empty blocks as part of the drag image.
Selecting everything and attempting to drag would drag an image that was the entire page, and you wouldn't be able to see what you were doing. 
But with the current patch, if you have a page full of text (quite common) and you select all, you end up dragging essentially the entire page, same problem, right? We should address that problem by scaling down oversized drag images.

> Otherwise, you'd get huge empty blocks as part of the drag image.

In what cases would this happen? When the user selects a complete block of text (i.e. a paragraph) whose text is much less wide than the width of the containing block? (With my proposal, only blocks that are entirely inside the selection would have their overflow areas included, so blocks that just contain the selection don't get involved.) That's a bit of a problem but maybe not so common; typically pages reduce the width of the containing block to something reasonable because long lines are hard to read, and typically paragraphs do reach the right margin.
I didn't do any optimization. It would be good to compare the two methods by dragging the selection on various pages.

I personally believe that including the block rectangles makes the appearance of the drag image just look more buggy, whereas not including them and just drawing the text (and improving the code so as not to draw the background either) even though it doesn't draw any borders in inline styles which might be part of the dragged data just looks intentional.
Attachment #257413 - Flags: superreview?(roc)
Attachment #257413 - Flags: review?(roc)
How about we just draw precisely the frames that are in the selection, and no others, with a transparent background, and make the selection region be the union of the display item rectangles? Or since we're being transparent anyway it could just be the union of the display item rectangles. Then when the selection contains blocks without borders or backgrounds, they'll effectively be ignored. This shouldn't be too hard actually and it would also solve the issue of dragging selected content that's underneath other content.

If you like this idea, you could do it like this:
1) Find the frame for the content that's the nearest common ancestor to the start and end nodes for all ranges in the selection
2) Build an *unoptimized* display list using that frame as the root
3) For each item in the display list, check whether its frame's content is in a selection range, and if it is not, throw it away. (This might be best done by first building a hashtable-set of the primary frame for each content node in the selection.) Here also any text display items should be trimmed to just the selection. Here you can also collect the bounding box of the display items.
4) Allocate your temporary surface and paint the display list to the surface. In this step if the bounding box is too large you can easily allocate a smaller surface and scale down while drawing.

This approach also means that if you select something that's clipped out or scrolled out of view by a container, it will still be rendered for dragging.
Actually things would probably go better if we built one display list for each selection range and then combined them.
Attachment #256498 - Flags: superreview?(roc)
Attachment #256498 - Flags: review?(roc)
Attachment #257413 - Attachment is obsolete: true
Attachment #257413 - Flags: superreview?(roc)
Attachment #257413 - Flags: review?(roc)
This patch uses display lists to draw the drag feedback for selections. See nsSelection::PaintSelection. It also scales large selections down based on the area they occupy.

The display lists are modified so that the text frames at the end points of a range are clipped, and any items outside of the range are removed.
Attachment #256498 - Attachment is obsolete: true
Attachment #258258 - Flags: superreview?(roc)
Attachment #258258 - Flags: review?(roc)
Basically this is fabulous.

+       The point of the
+     * aScreenRect rectangle should be filled in by the caller, generally
+     * to the screen position of the mouse. This rectangle will be modified
+     * by this method to hold the rectangle of the selection area on screen.

Can you use two parameters, an input point and an output rect?

+struct selectionPaintInfo {

SelectionPaintInfo. And please follow the m* naming scheme for fields.

+static void GetSelectionPaintInfoForDisplayList(nsDisplayListBuilder *aBuilder,

How about calling it ClipListToSelection?

+                                                nsPresContext* aPresContext,

Drop this parameter and just get it from the frame when you need it.

+                                                nsRect& aSurfaceRect)

How about returning the union of the rects for this list as the function result?

+  nsDisplayItem* stopItem = nsnull;

I'd really prefer that you used a temporary list instead of this trick. That would be simpler. AppendToTop(nsDisplayList*) is really cheap, so there's no performance advantage to doing it this way.

+  if (ancestor->IsNodeOfType(nsINode::eCONTENT))
+    ancestorContent = NS_STATIC_CAST(nsIContent*, ancestor);
+  if (ancestorContent) {

Why not just
  if (!ancestor->IsNodeOfType(nsINode::eCONTENT))
    return info;

How can the selection common ancestor not be content? Should there be an assertion here?

+    if (ancestorFrame && ancestorFrame->GetType() == nsGkAtoms::textFrame) {
+      do {
+        ancestorFrame = ancestorFrame->GetParent();
+      } while (ancestorFrame && ancestorFrame->GetNextInFlow());
+    }

I would just do
    while (ancestorFrame && ancestorFrame->GetNextInFlow()) {
      ancestorFrame = ancestorFrame->GetParent();
    }
The need to handle continuations is not specific to text frames.

There is a problem here --- out of flow frames won't get painted when the selection contains the placeholder but the containing block for the out of flow frames is not part of the selection. E.g. if you select some inline text that contains a float. I'll attach a patch that provides a SetPaintAllFrames method on the nsDisplayListBuilder (completely untested!) that should fix this.

+  // The area is checked rather than the width and height
+  // individually as it is common to have selections that are wide yet
+  // short

I don't get this. If I have a selection that's 10 pixels high and 20,000 pixels wide I'd like it scaled, right?

+    // adjust the screen position of the root frame to account for scrolling

I don't understand the logic here. Why is the root frame and its scroll position relevant?

+  if (!surface) {
+    for (PRInt32 i = rangeItems.Length(); i >= 0; i--) {
+      rangeItems[i]->list.DeleteAll();
+      delete rangeItems[i];
+    }
+    return NS_ERROR_FAILURE;
+  }

How about putting list.DeleteAll() in the destructor for the SelectionPaintInfo and making your nsTPtrArray an nsTArray<nsAutoPtr<SelectionPaintInfo> >?

+  context.SetOperator(gfxContext::OPERATOR_SOURCE);
+  context.SetColor(NS_RGBA(255, 255, 255, 0));

Use OPERATOR_CLEAR instead.

+    for (nsDisplayItem* i = (rangeInfo->list).GetBottom(); i != nsnull; i = i->GetAbove())
+      i->Paint(builder, rc, i->GetBounds(builder));

Call nsDisplayList::Paint

+    rc->Translate(-rangeInfo->rootOffset.x, -rangeInfo->rootOffset.y);

Use nsIRenderingContext::AutoPushTranslation

Why are we still calling nsIViewObserver::Paint from DrawDrag? Can't we use the same drawing path you just added for selections, or nearly so? Maybe create a temporary selection that contains just aDOMNode, or something like that? It seems like that would give better results and we could get rid of some of this code in nsBaseDragService.
Attached patch Incorporate feedback. (obsolete) — Splinter Review
This patch moves most of the drawing code from basedragservice and selection into presshell (alongside RenderOffscreen). When dragging a node, a range is created containing that node, so that both selection and node drawing code can be shared.

The patch in https://bugzilla.mozilla.org/attachment.cgi?id=258260 also helps when the selection contains absolutely positioned frames.

For the size restrictions, I added another check so that images that are larger than the presshell width or height are scaled down as well.

I removed the loop that calls Paint. The new code isn't any simpler though as now the bounds have to be determined for each range.
Attachment #258258 - Attachment is obsolete: true
Attachment #258439 - Flags: superreview?(roc)
Attachment #258439 - Flags: review?(roc)
Attachment #258258 - Flags: superreview?(roc)
Attachment #258258 - Flags: review?(roc)
+  NS_IMETHOD RenderNode(nsIDOMNode* aNode,
+                        nsIRegion* aRegion,
+                        nsPoint& aPoint,
+                        nsRect* aScreenRect,
+                        gfxASurface **aSurface);
+
+  NS_IMETHOD RenderSelection(nsISelection* aSelection,
+                             nsPoint& aPoint,
+                             nsRect* aScreenRect,
+                             gfxASurface **aSurface);

I think these should just return already_AddRefed<gfxASurface> directly. Ditto for PaintRangePaintInfo.

+   * aScreenRect will be filled in with the rectangle of the selection area
+   * on screen.

Bounding rectangle, right?

+  // retrieve a RangePaintInfo for the range aRange containing the
+  // display list needed to paint to range to a surface
+  RangePaintInfo* GetRangePaintInfo(nsIDOMRange* aRange,

Shouldn't this be CreateRangePaintInfo with comment "create" instead of "retrieve"?

+  // now add all the items back onto the original list again
+  while ((i = tmpList.RemoveBottom()))
+    aList->AppendToTop(i);

Just do aList->AppendToTop(&tmpList);

+  // go up the frame tree so that continuations are included

I would say
  // Use the nearest ancestor frame that includes all continuations as the
  // root for building the display list

+  if (ancestorFrame) {
+    do {
+      ancestorFrame = ancestorFrame->GetParent();
+    } while (ancestorFrame && ancestorFrame->GetNextInFlow());
+  }

Don't we actually want
  while (ancestorFrame && ancestorFrame->GetNextInFlow()) {
    ancestorFrame = ancestorFrame->GetParent();
  }
?

It's simpler and I think we don't need or want to always take one step up the parent chain.

+  // if the area of the image is larger than the maximum area, scale it down
+  float scale = 0.0;
+  nsIntRect rootScreenRect = GetRootFrame()->GetScreenRect();
+  PRBool resize = (pixelArea.width * pixelArea.height >
+                   MAX_RENDER_IMAGE_SIZE * MAX_RENDER_IMAGE_SIZE);
+  PRInt32 maxWidth = MAX_RENDER_IMAGE_SIZE;
+  PRInt32 maxHeight = MAX_RENDER_IMAGE_SIZE;
+  if (!resize) {
+    // if the image is larger in one or both directions than the size of the
+    // presshell's visible area, scale the image down to that size.
+    nsRect maxSize = pc->GetVisibleArea();
+    resize = (aArea.width > maxSize.width || aArea.height > maxSize.height);
+    if (resize) {
+      maxWidth = pc->AppUnitsToDevPixels(maxSize.width);
+      maxHeight = pc->AppUnitsToDevPixels(maxSize.height);
+    }

Why do you want to do it this way? I think we should just scale down until the width and height are both less than MAX_RENDER_IMAGE_SIZE.

How about something like
  float scale = 1.0;
  if (pixelArea.width > MAX_RENDER_IMAGE_SIZE) {
    scale = PR_MIN(scale, float(MAX_RENDER_IMAGE_SIZE)/pixelArea.width);
  }
  if (pixelArea.height > MAX_RENDER_IMAGE_SIZE) {
    scale = PR_MIN(scale, float(MAX_RENDER_IMAGE_SIZE)/pixelArea.height);
  }
and then update pixelArea and aScreenRect?

Do you happen to know what the region parameter for invokeDragSession is used for? I'm wondering why we need to support region-clipping of node rendering.

+      dest[2] = src[0];
+      dest[1] = src[1];
+      dest[0] = src[2];
+      dest[3] = src[3] * 0.8; // reduce transparency overall

Does this work on Intel and PPC Macs?

We should merge MAX_IMAGE_SIZE and MAX_RENDER_IMAGE_SIZE somehow. Maybe they could both be (the same) pref?
Also, please include the display list changes I suggested in your patch. We don't want to accidentally break the build.
> 
> Why do you want to do it this way? I think we should just scale down until the
> width and height are both less than MAX_RENDER_IMAGE_SIZE.

I had assumed that we don't want to scale one line of text, which on a large display, may be 1800-2000 pixels. Maybe we should anyway?

> 
> Do you happen to know what the region parameter for invokeDragSession is used
> for? I'm wondering why we need to support region-clipping of node rendering.

The region parameter is used when dragging tree rows, which have only one frame for the entire tree body and set the clip region to the rectangles of the selected rows.
(In reply to comment #67)
> > Why do you want to do it this way? I think we should just scale down until the
> > width and height are both less than MAX_RENDER_IMAGE_SIZE.
> 
> I had assumed that we don't want to scale one line of text, which on a large
> display, may be 1800-2000 pixels. Maybe we should anyway?

We should either scale it or make the MAX_(RENDER_)IMAGE_SIZE depend on the screen size.

> > Do you happen to know what the region parameter for invokeDragSession is used
> > for? I'm wondering why we need to support region-clipping of node rendering.
> 
> The region parameter is used when dragging tree rows, which have only one frame
> for the entire tree body and set the clip region to the rectangles of the
> selected rows.

OK. That makes sense. Thanks!
(In reply to comment #68)
 
> We should either scale it or make the MAX_(RENDER_)IMAGE_SIZE depend on the
> screen size.
> 

Yes, I'm using the presshell's size. If the area is wider or taller than it, I scale it down to be no more than that width/height.

> Also, please include the display list changes I suggested in your patch.
> We don't want to accidentally break the build.

Did you want someone to review it?
Attachment #258439 - Flags: superreview?(roc)
Attachment #258439 - Flags: superreview+
Attachment #258439 - Flags: review?(roc)
Attachment #258439 - Flags: review+
Comment on attachment 258439 [details] [diff] [review]
Incorporate feedback.

I'm not quite sure why I did that. Sorry!
Attachment #258439 - Flags: superreview+
Attachment #258439 - Flags: review+
The presshell size may not be reliable --- it could be very large or very small. I suggest using the screen size instead. And not use a hardcoded max size at all.

I'll get dbaron to review the display list patch. I assume it worked without modifications.
Comment on attachment 258260 [details] [diff] [review]
suggestion for SetPaintAllFrames

Add a little "paint all frames regardless of the dirty rect" feature.

Note that you can't get this effect just by setting a very large dirty rect, because some frames clip the dirty rect.
Attachment #258260 - Flags: superreview?(dbaron)
Attachment #258260 - Flags: review?(dbaron)
Comment on attachment 258260 [details] [diff] [review]
suggestion for SetPaintAllFrames

You might consider using field widths on the PRPackedBools, since there are now 5 of them.

r+sr=dbaron
Attachment #258260 - Flags: superreview?(dbaron)
Attachment #258260 - Flags: superreview+
Attachment #258260 - Flags: review?(dbaron)
Attachment #258260 - Flags: review+
Depends on: 372631
Attached patch more feedbackSplinter Review
I wasn't sure whether I should change nsBaseDragService::DragDrop to return the surface or to return an nsresult, so I didn't change these.

Changed to just scale the drag area to be no larger than half the screen size.
Attachment #258439 - Attachment is obsolete: true
Attachment #259001 - Flags: superreview?(roc)
Attachment #259001 - Flags: review?(roc)
Comment on attachment 259001 [details] [diff] [review]
more feedback

I don't much like the duplication of the scaling logic, but factoring it out is probably more trouble than it's worth.

Let's do it! woohoo!
Attachment #259001 - Flags: superreview?(roc)
Attachment #259001 - Flags: superreview+
Attachment #259001 - Flags: review?(roc)
Attachment #259001 - Flags: review+
The patch in bug 372631 is needed before this can be checked in, otherwise we'll get crashes trying to drag any text that contains ligatures.

 
Blocks: 374593
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 374832
Depends on: 375038
So when I drag an e-mail in mailnews (on Linux) it's dragged as fully opaque, which makes it hard to see where I'm dropping it.  Same thing when I drag an image (e.g. the top image on http://lxr.mozilla.org).  Looking at the code, I don't seem to see any provisions for translucency when dragging in the XP code, nor in the GTK2 code... am I missing something?

Note that on Mac we use 0.65 opacity, but that's in the mac-specific code (bug 374832).
If transparency is used on GTK, the image has random artifacts on it, so I didn't enable it. I should file a follow up bug so that it doesn't get forgotten.
The problem is that dragging opaque things is really really hard.  I've dropped several emails in the wrong folders today...

Is there a pref or something to disable this locally until we have the translucency working?
Depends on: 376071
Blocks: 376238
Depends on: 376578
Depends on: 378871
Depends on: 425069
Blocks: 425069
No longer depends on: 425069
Blocks: 439565
You need to log in before you can comment on or make changes to this bug.