Bug 142121 - Cell focus rectangle must not use font color
Summary: Cell focus rectangle must not use font color
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Natalia
URL:
Whiteboard: target:7.3.0 inReleaseNotes
Keywords: difficultyBeginner, easyHack, skillCpp, topicDesign
: 98176 142327 142959 145744 (view as bug list)
Depends on:
Blocks: Cell-Selection Options-Dialog-Colours
  Show dependency treegraph
 
Reported: 2021-05-06 10:05 UTC by Heiko Tietze
Modified: 2022-04-13 00:46 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments
dark frame (14.57 KB, image/png)
2021-05-17 11:01 UTC, steve
Details
A screen of the result of my uploaded patch (5.10 KB, image/png)
2021-09-25 18:08 UTC, Natalia
Details
Screenshot with the problem (1.02 KB, image/png)
2021-09-29 17:06 UTC, Roman Kuznetsov
Details
Screen with the fix for the black square (1.39 KB, image/png)
2021-10-09 05:37 UTC, Natalia
Details
verified 2021-10-11 (1.32 MB, image/png)
2021-10-11 09:09 UTC, steve
Details
Apple Numbers (151.82 KB, image/png)
2021-10-11 09:09 UTC, steve
Details
Calc 7.2 current cell (15.93 KB, image/png)
2022-02-26 16:47 UTC, Robert Kisteleki
Details
Calc 7.3 current cell (14.24 KB, image/png)
2022-02-26 16:47 UTC, Robert Kisteleki
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Heiko Tietze 2021-05-06 10:05:39 UTC
The focused cell has a black rectangle that nicely shows the active cell with the default color theme taking the value from Tools > Options > LibreOffice > Application Colors: Font Color (under General).

But for dark themes, eg LibreOffice Dark from bug 141986, it still is black- while the Automatic color is transposed into white. See https://bug-attachments.documentfoundation.org/attachment.cgi?id=171685 or switch Document Background to black.

IMO, the best color for the cell in focus is the same as we use on the row/col headings taken from the OS/DE. needsUXEval for this idea.
Comment 1 Heiko Tietze 2021-05-17 10:53:51 UTC
*** Bug 142327 has been marked as a duplicate of this bug. ***
Comment 2 steve 2021-05-17 11:01:37 UTC
Created attachment 172082 [details]
dark frame
Comment 3 Adolfo Jayme Barrientos 2021-08-28 14:26:57 UTC
Yes, yes! Back in the day, Mirek Mazel created a mockup where the OS highlight color was used for the selected cell frame (see https://wiki.documentfoundation.org/images/3/34/Calc-headers.png).
Comment 4 Heiko Tietze 2021-09-02 07:25:23 UTC
*** Bug 142959 has been marked as a duplicate of this bug. ***
Comment 5 Heiko Tietze 2021-09-02 07:28:48 UTC
Using the highlight color now for the col/row header, the actual selection, and - new in this patch - also the focused cell rectangle. Decided against an option since later the system highlight color might become part of the configuration itself and all places with a selection take the respective color.
Comment 6 Heiko Tietze 2021-09-13 04:50:11 UTC
Use the functional patch at https://gerrit.libreoffice.org/c/core/+/121491 as prototype.
Comment 7 Natalia 2021-09-25 18:08:44 UTC
Created attachment 175273 [details]
A screen of the result of my uploaded patch

I have attached a screenshot of how it now works after uploading the patch.
Comment 8 Natalia 2021-09-27 05:54:56 UTC
Gerrit link https://gerrit.libreoffice.org/c/core/+/122602
Comment 9 Commit Notification 2021-09-27 07:29:49 UTC
Natalia Gavrilova committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/b1c318e9f18f6a4e56d42fc7b7c5c2942b28941d

tdf#142121 Cell focus rectangle does not use font color

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 Heiko Tietze 2021-09-27 07:32:08 UTC
(In reply to Heiko Tietze from comment #6)
> Use the functional patch at https://gerrit.libreoffice.org/c/core/+/121491
> as prototype.

The issue in this report is solved. However, to make the selection more outstanding in zoomed view I added a few lines of code to adjust the line size according the zoom level. Looks good in my opinion but might have consequences ion performance (unlikely, though) and is unrelated to this issue. So if someone is interested in this enhancement, the prototype shows how to do.
Comment 11 Adolfo Jayme Barrientos 2021-09-27 09:12:20 UTC
I love how this enhancement looks. Thank you, Natalia!
Comment 12 Natalia 2021-09-27 09:36:27 UTC
Glad to hear!
Comment 13 Roman Kuznetsov 2021-09-29 16:46:33 UTC
After this patch I see a blue line around selected cell with BLACK square in its bottom right corner (I use a standard theme in LO)

Natalia, could you please fix it?
Comment 14 Natalia 2021-09-29 16:51:33 UTC Comment hidden (obsolete)
Comment 15 Natalia 2021-09-29 16:51:58 UTC Comment hidden (obsolete)
Comment 16 Roman Kuznetsov 2021-09-29 17:06:24 UTC
Created attachment 175376 [details]
Screenshot with the problem
Comment 17 Roman Kuznetsov 2021-09-29 17:07:01 UTC
(In reply to Roman Kuznetsov from comment #16)
> Created attachment 175376 [details]
> Screenshot with the problem

It's just LibreOffice in Windows 7
Comment 18 Natalia 2021-09-29 17:11:20 UTC
Do I understand correctly that expected behavior is to get this rectangle to be also highlight color instead of black (I suppose it’s font color)?
Comment 19 Heiko Tietze 2021-09-30 07:47:12 UTC
(In reply to Natalia from comment #18)
> Do I understand correctly that expected behavior is to get this rectangle to
> be also highlight color instead of black (I suppose it’s font color)?

Yes. Do you need a code pointer?
Comment 20 Natalia 2021-09-30 08:29:12 UTC
Hi, if you have that, that would probably be the easiest.
Comment 21 Heiko Tietze 2021-10-08 08:17:19 UTC
Sorry for the delay. The grip is drawn in ScGridWindow::UpdateAutoFillOverlay() with currently Color aHandleColor = svtools::FONTCOLOR.
Comment 22 Natalia 2021-10-09 05:36:02 UTC
New patch https://gerrit.libreoffice.org/c/core/+/123300
Comment 23 Natalia 2021-10-09 05:37:27 UTC
Created attachment 175607 [details]
Screen with the fix for the black square

The black square in the down right corner is also highlight color now
Comment 24 Commit Notification 2021-10-09 08:27:43 UTC
Natalia Gavrilova committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/c307c34a295fbbaf0af91a17440b045133981cc8

tdf#142121 Cell focus rectangle does not use font color

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 25 steve 2021-10-11 09:09:06 UTC
Verified - thanks, as this is a really nice small UI improvement that makes UI feel more consistent and at home on the various OSs.

Is it intended behavior to draw the bold focus rectangle only around the first top left cell when multiple cells are selected (see verified 2021-10-11 screenshot)? This feels wrong and Apple Numbers indeed puts the bolder line around *all* selected cells (see attachment Apple Numbers). Thoughts? Should be file a separate follow up bug or do we think current behavior is expected and fine?
Comment 26 steve 2021-10-11 09:09:29 UTC
Created attachment 175655 [details]
verified 2021-10-11
Comment 27 steve 2021-10-11 09:09:46 UTC
Created attachment 175656 [details]
Apple Numbers
Comment 28 Heiko Tietze 2021-10-11 12:57:40 UTC
(In reply to steve from comment #25)
> Is it intended behavior to draw the bold focus rectangle only around the
> first top left cell when multiple cells are selected...

Yes, Apple Numbers don't let you spot the currently active cell (bold frame).
Comment 29 fml2 2021-10-11 15:17:29 UTC
About "currently active cell" there is the bug#144593.
Comment 30 Buovjaga 2021-10-11 15:22:23 UTC
(In reply to al.le from comment #29)
> About "currently active cell" there is the bug#144593.

Moving the focus when selecting columns does not have anything to do with this report, removing from See Also.

I agree with Heiko that it is Apple Numbers that should change its behaviour.
Comment 31 steve 2021-10-11 18:42:43 UTC
Thanks for the input, so current behavior would be expected.

File a followup bug about contrast being too low for cell selection highlight color: https://bugs.documentfoundation.org/show_bug.cgi?id=145080
Comment 32 Rafael Lima 2021-11-22 12:25:49 UTC
Thanks for this UI improvement.

I added an entry in the release notes to highlight this new behavior.
Comment 33 Adolfo Jayme Barrientos 2021-11-29 08:29:20 UTC
*** Bug 145744 has been marked as a duplicate of this bug. ***
Comment 34 stragu 2022-01-01 11:42:03 UTC
Reviewing 7.3 release notes.

Verified in:

Version: 7.3.0.1 / LibreOffice Community
Build ID: 840fe2f57ae5ad80d62bfa6e25550cb10ddabd1d
CPU threads: 4; OS: Linux 5.4; UI render: default; VCL: gtk3
Locale: en-AU (en_AU.UTF-8); UI: en-US
Calc: threaded
Comment 35 fml2 2022-01-01 13:51:03 UTC
I've read the description of this feature on https://wiki.documentfoundation.org/ReleaseNotes/7.3 and I find the feature not good. The cursor can barely be seen IMO. The contrast is too low.
Comment 36 steve 2022-01-02 00:11:40 UTC
@al.le@gmx.de: Think everybody agrees about contrast being too low and visibility bad. This feature does not feel finished.

There are two follow up bugs about that:

- https://bugs.documentfoundation.org/show_bug.cgi?id=145080
- https://bugs.documentfoundation.org/show_bug.cgi?id=143733
Comment 37 fml2 2022-01-10 21:34:42 UTC
OK, I just hope the feature in its current form won't make it into the official release 7.3.
Comment 38 Buovjaga 2022-01-24 09:10:56 UTC
*** Bug 98176 has been marked as a duplicate of this bug. ***
Comment 39 Robert Kisteleki 2022-02-26 16:47:08 UTC
Created attachment 178561 [details]
Calc 7.2 current cell
Comment 40 Robert Kisteleki 2022-02-26 16:47:38 UTC
Created attachment 178562 [details]
Calc 7.3 current cell
Comment 41 Robert Kisteleki 2022-02-26 16:48:03 UTC
(In reply to fml2 from comment #37)
> OK, I just hope the feature in its current form won't make it into the
> official release 7.3.

It seems like the change made it as is. It may work well with a dark theme, but IMO this behavioural change has drawbacks, namely that now it's *really hard* to see what cell is selectd with some highlight colors. In my case - even though I don't have accessibility problems - the hichighted cell is almost invisible in a big enough window. See attachments - one for 7.2 and one for 7.3

*Please* at least put this behind a setting. As it is, I have to revert to use 7.2 because ther I can see which cell is selected.
Comment 42 Timur 2022-02-27 08:37:39 UTC
I don't follow the bug, but based on users' comments, commit should be reverted from 7.3. 
There were similar situations before and rule for visible changes should be: do not commit until all is done.
Comment 43 Buovjaga 2022-02-27 11:35:55 UTC
(In reply to Timur from comment #42)
> I don't follow the bug, but based on users' comments, commit should be
> reverted from 7.3. 
> There were similar situations before and rule for visible changes should be:
> do not commit until all is done.

What needs to be done is to look into using ifdef on macOS < 10.14, so this feature is not active. For macOS >= 10.14, the feature can use "accent color", which is discussed in bug 145080.
Comment 44 Mike Kaganski 2022-03-05 19:43:37 UTC
Natalia, * whoever develops on macOS: could you please consider bug 142121 comment 22, and implement retrieving the *available* accent color from system where it's available?

1. Implement a new GetSettings().GetStyleSettings().GetAccentColor(), that would for now default to GetSettings().GetStyleSettings().GetHighlightColor() on all systems except macOS;
2. On macOS, implement it using the weakly linked guarded call to controlAccentColor;
3. Use it where Natalia made the changes (so all the code pointers make it an easy hack).

No target version bump is required for that.