26831 - Implement table and cell selection as border highlighting instead of content highlighting

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Charles Manske]

When we select entire cells or the entire table, we want the border to be
highlighted, like the blue border we used earlier. Currently, selecting a cell
does the usual text selection style on the cell's contents.

Comment 1

[Kathleen :Brade]

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

Comment 2

[Kathleen :Brade]

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

Comment 3

[Garth Wallace]

Neither of those bugs are duplicates of this one. They are, however, duplicates
of each other.

Bug #29393 and Bug #29476 both refer to being able to select rectangular areas
of a table. This bug is about how selections should be highlighted.

Comment 4

[Kathleen :Brade]

actually they are duplicates; if you get the blue selection border then you'll 
have your rectangular selection.

Comment 5

[rubydoo123]

updating keyword and status whiteboard to reflect that this is a beta 2 feature 
work bug that the Composer team deems a must fix for beta 2.

Comment 6

[mjudge]

fixed, style color will be implemented when they get to css3 pseudo style

Comment 7

[kinmoz]

I just tested this in my Win32 10/27/00 Netscape_20000922_BRANCH build. I don't
see any border highlighting (rectangle drawn around the cell or table) ... if I
control click on a cell the entire cell background gets filled in with the
hilight color. Control clicking on an outside table border changes the
background of all cells to the hilight color.

Was border highlighting disabled in favor of this new method?

Comment 8

[sujay]

I also see the same behavior mentions in my 10/30 build...REOPENING

Comment 9

[Kathleen :Brade]

moving off M16 since that was long ago...

Comment 10

[Charles Manske]

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

Comment 11

[Charles Manske]

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

Comment 12

[Charles Manske]

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

Comment 13

[Charles Manske]

Note that this problem is frequently encountered as a "can't set table or cell
background color" problem. The number of bug duplicates supports this.
Let's please try to get this fixed for 0.9.

Comment 14

[Matthew T (active 1999-2002)]

If you use border highlighting instead of background highlighting, I imagine 
you'll have two new problems. (1) Instead of getting `Can't set table/cell 
background color' bugs, you'll start to get `Can't set table/cell border' bugs. 
(2) In large tables, especially where part of the selection is outside the 
window, it will be very difficult to tell what is selected and what isn't.

I suggest using a pixel-checkerboard pattern of the selection color to cover the 
cell background.
X X X X
 X X X X
X X X X
 X X X X
That way the cell/table will obviously be highlighted, but the existing 
background will still be visible. (The same thing applies for selecting images, 
bug 14835.)

Comment 15

[Charles Manske]

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

Comment 16

[shrirang khanzode]

is bug 50794 a dup of this ?

Comment 17

[mjudge]

This is a no-win situation.  The background highlighting for table cell 
selection is a good method.  the only side affect to this is the temporary 
confusion that the background color did not take.  moving to .091 for now

Comment 18

[timeless]

could we use a hatched pattern? background overlay a png file
<pattern>
X0
0X
</pattern>
X is translucent, O is 100% transparent.

Comment 19

[mjudge]

ok here are some patches. we can flip switch back easily if people get upset at 
the result

Comment 20

[mjudge]

Attachment: patch 1 for layout/html/table/src

Comment 21

[mjudge]

Attachment: patch 2 for content/base/src

Comment 22

[mjudge]

kin allready reviewed this so r=kin if he doesnt add that to this bug himself

Comment 23

[Chris Waterson]

(cc'ing karnze, who may have other comments on nsTableCellFrame.)

In nsTableCellFrame.cpp, it seems like this whole big mess should be moved out
to a separate function. The nesting is starting to walk off the right edge of my
screen. What do you think?

Probably don't need to check in the commented out stuff:

+            if (NS_SUCCEEDED(result) && tableCellSelectionMode) {
+              //frameSelection->GetTableCellSelectionStyleColor(&myColor); 

I think spaces around `==' are prevalent style in this file.

+              if(displaySelection==nsISelectionController::SELECTION_DISABLED) {

Shouldn't this come from nsILookAndFeel?

+                bordercolor = NS_RGB(176,176,176);// disabled color

Usually, people call this `t2p' for twips-to-pixels:

+              float twipsfloat;
+              if (NS_SUCCEEDED(aPresContext->GetPixelsToTwips(&twipsfloat)))
+              {
+                PRInt16 twipsperpixel = (PRInt16)twipsfloat;

Are you sure that myColor is initialized by this point? (I didn't look carefully
at the code preceding the patch.)

+                  //compare bordercolor to ((nsStyleColor
*)myColor)->mBackgroundColor)
+                  bordercolor = EnsureDifferentColors(bordercolor,
myColor->mBackgroundColor);

In nsSelection.cpp,

I think you want ``#undef OLD_SELECTION'' here:

+#define OLD_SELECTION 0
+//#define OLD_TABLE_SELECTION 1

Comment 24

[kinmoz]

Also, just a note on the number of calculations that are done in the patch:

We can reduce 6 muliplications (2*twipsperpixel) down to one with the use of a 
temporary variable.

Same goes for the various (width - twipsperpixel), (height - twipsperpixel), 
(width - 2*twipsperpixel), (height - 2*twipsperpixel) values which are used more 
than once.

Comment 25

[kinmoz]

Also, I didn't see any comments/reply to timeless and mpt's request for using 
hatched patterns instead. Should they file that as a seperate bug/RFE?

Comment 26

[Charles Manske]

I agree that if we can do a hatched pattern, it would be much better.
I'll help test as soon as I can get &%$# cvs to let me pull everything!

Comment 27

[mjudge]

as far as the review goes...

took out comment.
added in spaces around "==".
nsILookAndFeel doesnt have "disabled selection".
changed variable to t2p/t2pfloat.
mycolor is initialized and then asserted on. (I moved the assertion checks 
closer to assignment to be more clear there.)
I actually didnt add that OLD_SELECTION i just moved it. it uses #if not #ifdef 
to check. I have made it #ifdef and #undef to make it more clear.

added 2 methods to break out the functions from paint so paint looks neater.

Comment 28

[mjudge]

Attachment: patch 1.1 in layout/html/table/src

Comment 29

[mjudge]

Attachment: patch 2.1 in content/base/src

Comment 30

[mjudge]

ok so far as the hatch pattern..
i cant find a method that will allow a filled rectangle to be drawn with a 
pattern in an XP way. if anyone finds one let me know. 

we could do an "X" or vertical and horizontals to make like "#" but the 
diagonals are to much a pain to do just for nsTableCellFrame::Paint i think.  i 
want to check in this patch with the understanding that it is better than what 
it was and that we can argue over the exact pattern to be used.

Comment 31

[Chris Waterson]

Cool, thanks. Couple more nits! :-)

> nsILookAndFeel doesnt have "disabled selection".

Should it? (Seems like it should to me.) If so, please file another bug (maybe 
on XPToolkit?) If not, just tell me to go to my corner.

+      const nsStyleColor* myColor = (const 
nsStyleColor*)mStyleContext->GetStyleData(eStyleStruct_Color);
+#ifdef OLD_TABLE_SELECTION
+      myColor = GetColorStyleFromSelection(mycolor);
                                            ^^^^^^^

Did you mean my*C*olor (intercaps)? (It's ifdef'd out, so probably not caught by 
compile.)

Do you want to address kin's gripe about extra multiplies?

Looks good to me. sr=waterson

Comment 32

[timeless]

i was hoping you could just use a PNG, i don't actually know how.

Comment 33

[mjudge]

yeah mycolor should be myColor i actually changed that here allready when i 
tested it with ifdef on.  thanks.  png transparancy.. hmm let me talk to pavlov. 
either way i am going to check this in and then we can keep talking about the 
specifics.

yes lookandfeel could have disabled selection. let me see if i can talk to 
pierre before he leaves.

Comment 34

[mjudge]

resolving the bug. if you want to make a fancier pattern/ blending we need a new 
bug on this. talked to pav and until gfx2 goes in it doesnt look good for either 
right now.

Comment 35

[sujay]

verified in 5/30 build.