Last Comment Bug 735641 - No way to deselect image of image document after select all (Ctrl+A)
: No way to deselect image of image document after select all (Ctrl+A)
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 11 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mats Palmgren (vacation)
:
Mentors:
Depends on: 846943
Blocks: 376997
  Show dependency treegraph
 
Reported: 2012-03-14 05:45 PDT by Alice0775 White
Modified: 2013-03-02 19:34 PST (History)
14 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.06 KB, patch)
2012-04-12 08:17 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
fix+test (3.11 KB, patch)
2012-04-12 14:09 PDT, Mats Palmgren (vacation)
bzbarsky: review+
ehsan: feedback+
Details | Diff | Splinter Review

Description Alice0775 White 2012-03-14 05:45:27 PDT
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] 2012-03-18 15:43:19 PDT
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) 2012-03-20 12:52:33 PDT
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] 2012-03-21 01:27:44 PDT
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 2012-03-21 05:23:30 PDT
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] 2012-03-21 05:54:09 PDT
(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] 2012-03-21 06:51:36 PDT
Actually, even clicking on top of the selection normally clears it.
Comment 7 Neil Deakin 2012-03-21 08:36:02 PDT
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] 2012-03-26 00:30:54 PDT
(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 2012-04-10 19:53:16 PDT
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] 2012-04-11 18:29:18 PDT
(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 2012-04-11 18:42:47 PDT
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] 2012-04-11 19:15:29 PDT
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] (Mozilla Japan) 2012-04-11 19:33:29 PDT
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] 2012-04-11 19:42:22 PDT
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 (vacation) 2012-04-12 08:17:03 PDT
Created attachment 614379 [details] [diff] [review]
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 (vacation) 2012-04-12 14:09:43 PDT
Created attachment 614561 [details] [diff] [review]
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 [:bz] 2012-04-12 14:13:34 PDT
Comment on attachment 614561 [details] [diff] [review]
fix+test

r=me
Comment 19 Ed Morley [:emorley] 2012-05-04 10:55:56 PDT
https://hg.mozilla.org/mozilla-central/rev/12cb1f643adb

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