123727 - Clicking on an image in Composer doesn't select it.

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Charles Manske]

Open any page with an image or create one and insert an image.
Click on the image. It does not change the current selection -- it should 
select the image.

Comment 1

[Charles Manske]

*** Bug 124228 has been marked as a duplicate of this bug. ***

Comment 2

[sujay]

cc Tucson

also carrying over my steps from DUP bug:


1) launch netscape
2) launch composer
3) insert image
4) click on image to select it.

notice:

a) no blue border showing selection
b) enter some text next to image, then click on image, then type more
text, image never got selected, so they text you type keeps going instead
of replacing the image with text.

Kin also sees these problems.

definitely some regression in image selection.

Comment 3

[kinmoz]

I think we may unintentionally be picking up some style on images that is
causing the IsSelectable() call in nsFrame::HandlePress() to return false.

Can we figure out when this started happening? It might make it easier to see
what checkin caused this.

Comment 4

[sujay]

Tucson to investigate when this broke...

Comment 5

[michael wendell]

This problem first showed itself in build 2002020512.  The trunk builds before
it (02-04 and 02-02) were both not functional due to bug 123399, and builds
02-01 and before work fine.

Comment 6

[Kathleen :Brade]

I downloaded the Mac mozilla build from 2/4 (4am?) and it also has this problem
(unable to select images).

Are there builds from the middle of the night or any other time which might help
us narrow this down?

Comment 7

[kinmoz]

I did some quick debugging on this last week and found 2 problems. The first one 
is the IsSelectable() problem I mentioned above. The 2nd one is that this call 
in nsEditor.cpp:

    aSelCon->SetSelectionFlags(nsISelectionDisplay::DISPLAY_ALL);

seems to set the flag on a presShell, which is different from the presShell used 
when rendering. That is when the layout rendering code goes to see if it should 
draw a box around the image because it is selected, it doesn't because the 
DISPLAY_ALL flag is not lit.

Mike are you looking into this one? It's a pretty big regression.

Comment 8

[Blake Ross]

Attachment: patch

Comment 9

[Simon Fraser [no longer active]]

It sucks that nsFrame has to know about editing. And what's with this (original) 
line:
   isEditor = isEditor == nsISelectionDisplay::DISPLAY_ALL;
?
I'll leave kin to comment further.

Comment 10

[kinmoz]

Comment on attachment 69170 [details] [diff] [review]
patch

The isEditor bool/flag should be renamed to something like
displaySelectionFlags and we should probably be looking for DISPLAY_IMAGES and
DISPLAY_FRAMES instead of assuming DISPLAY_ALL.

Shouldn't we be overriding HandlePress in image and area frame instead of the
generic frame class?

Comment 11

[saari (gone)]

-> blake, drive zee patch!

Comment 12

[kinmoz]

I'll sr=kin the first half of the patch since it will fix this bug, blake 
you'll need to rework/rethink the approach on how to get your desired D&D 
behavior.

Index: ./document/src/html.css
===================================================================
RCS file: /cvsroot/mozilla/layout/html/document/src/html.css,v
retrieving revision 3.143
diff -u -r3.143 html.css
--- ./document/src/html.css     3 Feb 2002 00:06:00 -0000       3.143
+++ ./document/src/html.css     13 Feb 2002 01:21:05 -0000
@@ -404,10 +404,6 @@
   cursor: pointer;
 }
 
-img, area {
-  -moz-user-select: none !important;
-}
-
 img[usemap], object[usemap] {
   color: blue;
   -moz-user-focus: normal;

Comment 13

[Håkan Waara]

Will that patch to html.css make images selectable in webpages too?

Comment 14

[Charles Manske]

Yes -- images are selectable in browser.
Blake: So I assume you changed this so you could click on image and 
immediately drag it? 

Comment 15

[Blake Ross]

I can't say I understand how the addition in the second part of the patch is at
all different from the code that comes right after it, which looks for a link.
Yes, perhaps this is better placed in nsImageFrame or whatever, but why did that
code get checked in in nsFrame::HandlePress, then?

Comment 16

[Blake Ross]

And what do you mean rethink/rework the approach? What are the other issues here
besides the placement? This patch seems correct to me.

Comment 17

[kinmoz]

I didn't add the link code, but I suppose it was added because all of the 
elements that can live under a link need to do the exact same thing.

In any case, We don't want to be adding special case code to the base class if 
we can help it. In your case we have specialized frames for the content you are 
concerned with.

Also, your nsFrame.cpp changes are using something which I thought was intended 
to modify "display of selection", not selection, to short circuit selection.

Cc'ing mjudge, during a meeting today he seemed to think there were other issues 
that needed to be considered.

Also what implications does shutting off selection on image and area have on 
accessibility?

Comment 18

[Blake Ross]

It does not shut off selection of images and areas, it shuts off *initiation* of
a selection when the press originates on an area or an image.  This is exactly
how IE behaves. I can't think of any accessibility implications, but cc'ing aaronl.

nsFrame::HandlePress is where we start the selection, so I think this is the
proper place to handle it.  This is also the place where we prevent dragging of
a link and instead initiate a selection if you hold down alt while selecting
within a link.  Now we just want to do the opposite -- prevent the selection,
let a drag start.

Comment 19

[Kathleen :Brade]

cc Aaron per Blake's previous comments

Comment 20

[Aaron Leventhal]

I'm more concerned about being able to select images and everything else using
the keyboard, which seems to be working. As far as mouse selection goes, and how
it relates to accessibility, that decision should be based on usability for
everyone.

Comment 21

[Blake Ross]

Attachment: patch

Comment 22

[Blake Ross]

Attachment: patch (with header file changes)

Comment 23

[mjudge]

I see how you are trying to let the editor continue to work.  Is is possible to 
simple disallow the click selection of images/area frames in all cases, then if 
the drag does not take place (i.e. you click then let go over the same image 
ect) that you return and select just that image.  

This will prevent the start of a continuous selection with an image or area 
frame.  I think thats ok.  It will also allow people to click on images in the 
browser then copy them to clipboard ect without selecting fromone side of it to 
the other.  

You could also remove the code (which we all hate but its been there forever) to 
check for the editor based on the selection flags ect.  just boot out of click 
selecting in images and areaframes period.

Comment 24

[Blake Ross]

Okay, but can you provide tips on how to do that...?  HandlePress is what starts
the selection but we don't know at that point if it's going to end up a click or
a drag.

Comment 25

[Kathleen :Brade]

r=brade on the html.css changes that kin sr'd
Can we get approval to check that in for 0.9.9?

Comment 26

[Kathleen :Brade]

add dependency; you can't constrain an image if you can't get to its dialog and
you can't get to a properties dialog if you can't do the initial selection.

Comment 27

[sujay]

I was able to get to image props by context menu....

Comment 28

[Kathleen :Brade]

remove dependency since QA found a workaround

By the way, I talked to Blake and he was under the mistaken impression that the
css change had already been backed out so that is why it isn't in his most
recent patch.  He is ok with the html.css change (backout) being checked in.

Comment 29

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9 for the html.css part

Comment 30

[Charles Manske]

backout of html.css change checked in.

Comment 31

[Boris Zbarsky [:bzbarsky]]

What is the effect of this change on the browser?  Does clicking on an image in
browser select it now?  More importantly, does clicking on an image in the
browser blow away the X Primary selection (as it used to at some point)?

Comment 32

[sujay]

if the file is backed out can we get a resolution on this bug? also
why is this bug marked 1.1 milestone?

Comment 33

[kinmoz]

This bug should be closed as FIXED and blake's original D&D bugs REOPENED.

Comment 34

[Charles Manske]

Dragging an image from Browser happens with on click down and drag; when in 
Composer you must click on image to select, then mouse down and drag, but that is
the correct behavior (same as in 4.x).
I see one problem with draggin image from browser: the SRC url of image is not
made absolute, so when you drop in composer, image is broken (and asserts) because
it is a relative URL. This doesn't happen when dragging from Composer window to
composer window. But maybe that is a separate bug from blakes?
I can't find any drag and drop bug for blake.

Comment 35

[sujay]

what is the status on this bug? 

Comment 36

[Kathleen :Brade]

selection of images in Composer is fixed (was fixed for 0.9.9 if I recall correctly)

I don't know the other bug Blake was working on that was partially backed out. 
That bug should be reopened.

Comment 37

[sujay]

verified in 3/11 build.