735641 - No way to deselect image of image document after select all (Ctrl+A)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/8d1c74566a0b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120313 Firefox/13.0a1 ID:20120313090404

In Firefox11 and later,
No way to deselect image of image document after select all (Ctrl+A).

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with Clean porofile
2. Open an image (ex. https://bugzilla.mozilla.org/skins/standard/index/help.png )
4. Ctrl + A
5. Try to deselect image

Actual Results:
 No way to deselect

Expected Results:
 Clicking border should deselect image

Regression range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dca42848a1e2&tochange=2a0b56de8e9e

Comment 1

[Christian Sonne [:cers]]

At least on mac, pressing the tab key twice deselects the image. Still clearly not an ideal situation.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I think this might have to do with the absolute positioning of the image, or at least that seems to me to be the only notable change from pre-376997.

Comment 3

[Justin Dolske [:Dolske]]

Yeah, I noticed this too. Not sure what's causing it.

A simple wallpaper might be to use -moz-user-select:none to prevent the selection in the first place?

Comment 4

[Neil Deakin]

There isn't a direct way with the keyboard to deselect things that aren't images either, apart from tabbing away. We don't have a 'Deselect All' command.

Comment 5

[Christian Sonne [:cers]]

(In reply to Neil Deakin from comment #4)
> There isn't a direct way with the keyboard to deselect things that aren't
> images either, apart from tabbing away. We don't have a 'Deselect All'
> command.

True, but try selecting some (or even all) text on the page, and then click somewhere that isn't on top of the selection. That clears the selection.

Comment 6

[Christian Sonne [:cers]]

Actually, even clicking on top of the selection normally clears it.

Comment 7

[Neil Deakin]

OK, I see the image document has been changed to display in a different way. I didn't realize that this was talking about a regression.

Some debugging shows that the difference between position: absolute and not causes nsFrame::DrillDownToSelectionFrame to retreive a different frame when looking to see if the mouse is over an existing selection. The issue that the absolute placeholder frame for the image is considered to be empty so isn't considered part of the selection I guess.

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to Justin Dolske [:Dolske] from comment #3)
> A simple wallpaper might be to use -moz-user-select:none to prevent the
> selection in the first place?
-moz-user-select:none didn't prevent Ctrl+A. Even worse, it prevented to deselect the selection!

Comment 9

[Dieter Hansen]

Maybe the solution I posted on the 376997 bug page could help you.
https://bugzilla.mozilla.org/show_bug.cgi?id=376997

Comment 10

[Justin Dolske [:Dolske]]

(In reply to Neil Deakin from comment #7)

> Some debugging shows that ...

Neil: should you own this? Or propose someone else?

Also, I just tried a quick workaround. If I use DOM Inspector to toss a text node into the document, deselection works properly. So perhaps we could just add a dummy space character? Doesn't fix the core problem, but gets it working properly.

Comment 11

[Neil Deakin]

Not me. Mats Palmgren or Masayuki might have more insight into the reasons behind comment 7.

The workaround sounds ok for image documents though.

Comment 12

[Justin Dolske [:Dolske]]

CCing mats and masayuki per last comment.

Also, Jared and I were just talking about this, and I can reproduce this with a standalone HTML document so it's not just somehow unique to ImageDocuments.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Why don't you try disabling select all command on image document? It seems that the command doesn't make sense on such situation. If it's need to copy, I think copy command should be always enabled and copy the image without selection. It's easier for users.

Comment 14

[Justin Dolske [:Dolske]]

That might be a workaround for the use case this bug was filed against. But I don't think that can be assumed for an arbitrary web page that hits this bug.

Comment 15

[Mats Palmgren (inactive)]

Attachment: fix

This hack seems to be the root of the problem, it makes
nsFrameSelection::TakeFocus *fail* if the new node is the root node.
I don't see any reason for this hack since the same thing can be done
by setting up the Selection that way using script.

The hack was added 2000-03-21:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.1&root=/cvsroot#1489

Comment 16

[Mats Palmgren (inactive)]

Attachment: fix+test

The code change did pass regression tests on Try:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=a8fc86074929

Results for the added mochitest is pending:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=68918378eaaf

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 614561 [details] [diff] [review]
fix+test

r=me

Comment 18

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12cb1f643adb

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/12cb1f643adb