If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in mozilla0.9.1

Status

()

Core
Editor
P1
major
VERIFIED FIXED
18 years ago
16 years ago

People

(Reporter: Charles Manske, Assigned: mjudge)

Tracking

Trunk
mozilla0.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Depends on: 26833
(Reporter)

Updated

18 years ago
Target Milestone: M15

Comment 1

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

Comment 2

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

Comment 3

18 years ago
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

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

Updated

18 years ago
OS: Windows NT → All
Hardware: PC → All
Target Milestone: M15 → M16

Comment 5

18 years ago
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.
Severity: normal → major
Keywords: beta2
Priority: P3 → P1
Whiteboard: Composer feature work
(Assignee)

Comment 6

18 years ago
fixed, style color will be implemented when they get to css3 pseudo style
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Updated

18 years ago
Keywords: nsbeta2

Comment 7

17 years ago
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

17 years ago
I also see the same behavior mentions in my 10/30 build...REOPENING
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

17 years ago
moving off M16 since that was long ago...
Target Milestone: M16 → mozilla0.9
(Reporter)

Comment 10

17 years ago
*** Bug 61278 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 11

17 years ago
*** Bug 53741 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 12

17 years ago
*** Bug 66360 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 13

17 years ago
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

17 years ago
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.)
(Reporter)

Comment 15

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

Comment 16

17 years ago
is bug 50794 a dup of this ?
(Assignee)

Comment 17

17 years ago
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
Status: REOPENED → ASSIGNED
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 18

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

Updated

17 years ago
Whiteboard: Composer feature work → Composer feature work [QAHP]
(Assignee)

Comment 19

17 years ago
ok here are some patches. we can flip switch back easily if people get upset at 
the result
(Assignee)

Comment 20

17 years ago
Created attachment 32929 [details] [diff] [review]
patch 1 for layout/html/table/src
(Assignee)

Comment 21

17 years ago
Created attachment 32930 [details] [diff] [review]
patch 2 for content/base/src
(Assignee)

Comment 22

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

Comment 23

17 years ago
(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

17 years ago
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

17 years ago
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?
(Reporter)

Comment 26

17 years ago
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!
(Assignee)

Comment 27

17 years ago
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.
(Assignee)

Comment 28

17 years ago
Created attachment 32995 [details] [diff] [review]
patch 1.1 in layout/html/table/src
(Assignee)

Comment 29

17 years ago
Created attachment 32996 [details] [diff] [review]
patch 2.1 in content/base/src
(Assignee)

Comment 30

17 years ago
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

17 years ago
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

17 years ago
i was hoping you could just use a PNG, i don't actually know how.
(Assignee)

Comment 33

17 years ago
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.
(Assignee)

Comment 34

17 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED
Whiteboard: Composer feature work [QAHP]

Comment 35

17 years ago
verified in 5/30 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.