Bug 111769 - Black is set as New color in Area tab when a table with multiple colors in its cells is selected
Summary: Black is set as New color in Area tab when a table with multiple colors in it...
Status: RESOLVED WONTFIX
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
5.3.0.3 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyInteresting, easyHack, skillCpp, topicUI
Depends on:
Blocks: ImpressDraw-Tables Area-Fill-Tab-Color
  Show dependency treegraph
 
Reported: 2017-08-13 16:01 UTC by Tamás Zolnai
Modified: 2019-04-01 08:29 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
Screenshot of the area tab (55.22 KB, image/png)
2017-08-13 16:04 UTC, Tamás Zolnai
Details
Table with different fillings (40.88 KB, image/png)
2017-10-23 07:12 UTC, Heiko Tietze
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tamás Zolnai 2017-08-13 16:01:45 UTC
Description:
In general when opening the Area tab and the selected object has a background color, then the color page of the area tab is opened and both the active and new color is set to the actual color of the selected object. But not in the case of a newly inserte table, where the new color is set to black.

It can cause a problem when you open the Table Properties dialog to change some settings and the area tab will be opened (because that was the last used page), you switch the tab and make some changes and when you click OK the background color will be changed to black.

Steps to Reproduce:
1. Open Impress
2. Insert a table
3. Table contect menu -> Table Properties -> Area tab

Actual Results:  
New color is set to black on area tab, color page.

Expected Results:
New color should be set to the same color as the active color is set.


Reproducible: Always

User Profile Reset: No

Additional Info:


User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36
Comment 1 Tamás Zolnai 2017-08-13 16:04:21 UTC
Created attachment 135520 [details]
Screenshot of the area tab

The picture shows an additional issue. The active color preview is OK, but RGB values means a black color. It might be related to the main issue.
Comment 2 Telesto 2017-08-13 16:11:22 UTC
Repro with:
Version: 6.0.0.0.alpha0+
Build ID: 46b4eb8b0e9325f8c29cd391baf9504bccee1837
CPU threads: 4; OS: Windows 6.19; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2017-08-11_06:44:17
Locale: nl-NL (nl_NL); Calc: CL
Comment 3 Tamás Zolnai 2017-08-15 00:49:58 UTC
I found out why is this issue is there. It comes up when selection contains more objects with different background fill. You can check it with selecting two cells with matching background color. In this case are tab shows good color. However if you select two cells with different color you get this bad behavior.
I suggest to make this work similar to Writer table backgrounds. When more different background is selected the none page of are tab should be displayed, but also do not apply any background if the user does not change the settings.
Comment 4 Tamás Zolnai 2017-08-15 00:59:48 UTC
Let's turn this into a easy hack. The code is here:
cui/source/tabpages/tparea.cxx
Check the SvxAreaTabPage class
Comment 5 Ekansh Jha 2017-10-18 09:37:40 UTC
Hello, 

Is this applicable to ubuntu also? because in ubuntu impress the i don't think the problem is same. I am selecting particular block in the table and then opening table properties, I found that, active color is always black no matter what is the actual active color and new color is always set to be blue. btw when i choose new color from the palette, i think its working fine. Should I add a screenshot to it?

If I have not understood the problem, please briefly describe it. 

Thank you
Comment 6 Yousuf Philips (jay) (retired) 2017-10-18 11:32:17 UTC
The issue boils down to that when multiple cells are selected that have different colors, then the dialog opens with black as the New color. In the old area fill dialog, Color fill type was toggled but no color in the listbox was pre-selected, but with the new area fill dialog introduced in 5.3, a New color would have to be active, but the question is, should it be black?

Heiko, Stuart, Cor: Whats your take?
Comment 7 Heiko Tietze 2017-10-18 11:44:21 UTC
Looks like the same issue as in bug 103224 and bug 108670. If the red-cross solution is not possible we could also go with the default color for this object, here tools > options > application color... and surprise surprise, all black in master. Should be white by default.

Version: 6.0.0.0.alpha0+
Build ID: b087e451527f2e497ccab83b63b4f10099bfb8b8
CPU threads: 4; OS: Windows 6.1; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2017-10-03_23:43:39
Locale: de-DE (de_DE); Calc: group
Comment 8 Tamás Zolnai 2017-10-18 14:23:52 UTC
(In reply to Heiko Tietze from comment #7)
> Looks like the same issue as in bug 103224 and bug 108670. If the red-cross
> solution is not possible we could also go with the default color for this
> object, here tools > options > application color... and surprise surprise,
> all black in master. Should be white by default.
> 
> Version: 6.0.0.0.alpha0+
> Build ID: b087e451527f2e497ccab83b63b4f10099bfb8b8
> CPU threads: 4; OS: Windows 6.1; UI render: default; 
> TinderBox: Win-x86@42, Branch:master, Time: 2017-10-03_23:43:39
> Locale: de-DE (de_DE); Calc: group

First of all we have a bit different issue here. In bug 108670 the question is what to display when click in Color tab of Area dialog. In the other hand, in this case the question is which tab to open inside Area dialog when selection has more fill colors. Now Color tab is opened by default with some weird colors. I suggest to open "None" tab instead in this case (when fill is ambigous).
It works similar in other cases. Check Table Properties -> Background in Writer. There "No fill" color is selected. You also can check Character -> Font color when you select text with different colors. It also displays "none" color. So I think it would be the best to be consistent here and display "None" when open Area dialog with an ambigous selection.
Comment 9 Yousuf Philips (jay) (retired) 2017-10-18 15:55:04 UTC
(In reply to Tamás Zolnai from comment #8)
> Now Color tab is opened by default with some
> weird colors. I suggest to open "None" tab instead in this case (when fill
> is ambigous).

I'd stick with Color tab, as a cell color was assigned to the cells, and as Heiko suggested, we have the red-cross in New color until a user decides to set one.

> It works similar in other cases. Check Table Properties -> Background in
> Writer. There "No fill" color is selected. You also can check Character ->
> Font color when you select text with different colors. It also displays
> "none" color. So I think it would be the best to be consistent here and
> display "None" when open Area dialog with an ambigous selection.

In Writer, when you open the table properties dialog, you are modifying the table-level properties and can set a single background color. In Impress, when you open the table properties dialog, you are opening a properties dialog based on what is currently selected and are not modifying table-level properties, which is why the background tab messes up when cells have multiple colors.
Comment 10 Ekansh Jha 2017-10-18 16:00:27 UTC
Is this bug open to work, if yes Please make it more understandable, I am getting a bit confused.
Comment 11 Tamás Zolnai 2017-10-18 16:11:48 UTC
> In Writer, when you open the table properties dialog, you are modifying the
> table-level properties and can set a single background color. In Impress,
> when you open the table properties dialog, you are opening a properties
> dialog based on what is currently selected and are not modifying table-level
> properties, which is why the background tab messes up when cells have
> multiple colors.

No, you are wrong here. Writer table properties dialog works the same. When cells are selected it uses the selected's cells color. So I still think it's the best to be consistent here and recognize ambigous fill attributes as "None".

Also this crossed-out image think might be harder to implement, but of course you can implement it. If you are stick with your idea (under you, I mean UX team), then better to remove easyhack keyword and I guess somebody can implement it in the UX team.
Comment 12 Tamás Zolnai 2017-10-18 16:12:43 UTC
(In reply to Ekansh Jha from comment #10)
> Is this bug open to work, if yes Please make it more understandable, I am
> getting a bit confused.

We are still discussing what would be the best, as you can see. After we have an agreement it will be more clear, what to do here.
Comment 13 Ekansh Jha 2017-10-18 16:32:48 UTC
I'll wait until then. 
Thanks
Comment 14 Tamás Zolnai 2017-10-18 17:49:12 UTC
One more thing. This ambiguity problem is not only affects Color fill, but also other types of background fills (e.g. hatcing, bitmap, etc.). So if we say in case of two different color fills open the Color tab, then what to open when we have a color fill and a pattern fill in the same selection?
Comment 15 Heiko Tietze 2017-10-18 17:55:31 UTC
Good points, Tamás. Mixed fill types cannot be represented in the dialog. But while any option is wrong "none" is also not correct. And it's more likely to have different colors than different types of filling. So I would simply use the filling of the first cell.
Comment 16 Tamás Zolnai 2017-10-18 18:28:56 UTC
The actual issue here is that what we select on area / background tab, it can override the actual fill properties of all selected cells.
Dialogs remember which tab page was opened last time and so it can happen that if you want to change other properties (e.g. font) of the selected cells, the area / background tab will be opened in the table properties dialog and so some color is set on color tab page which will be applied by pushing the OK button (when user's intention was to change only the font).
In case of "None" tab we would have the same result, setting all selected cells' background color to transparent. So an other option can be to not select any of the buttons on area / background tab, which visually is the same when "None" is selected except that the "None" button is not in pushed state. Without selection the dialog won't override the selected cells' properties unintentionally.
Comment 17 Yousuf Philips (jay) (retired) 2017-10-18 19:37:23 UTC
(In reply to Tamás Zolnai from comment #11)
> No, you are wrong here. Writer table properties dialog works the same. When
> cells are selected it uses the selected's cells color.

So let me clarify more what i meant by "you are modifying the table-level properties and can set a single background color". In Writer's table properties dialog's Background tab there is a 'For' drop down list that allows you to set the color of the table, row or cell level, while Impress's table properties only allows you to set the color at the cell-level.

> So I still think it's
> the best to be consistent here and recognize ambigous fill attributes as
> "None".

Well as one cell could have 'None', another could have 'Color' and another could be 'Bitmap', using 'None' is likely the best choice (meaning nothing has been universally set across all cells), but please ensure that if 'None' is selected, it isnt applied after visiting the tab.

> Also this crossed-out image think might be harder to implement, but of
> course you can implement it. If you are stick with your idea (under you, I
> mean UX team), then better to remove easyhack keyword and I guess somebody
> can implement it in the UX team.

I'm not a c++ dev to be able to implement it, but am sure that glade/vcl supports showing/hiding widgets and we could have the crossed-out image widget appear in particular circumstances and hide the color fill box widget. I'm fine with leaving it an easyhack and using 'None'.
Comment 18 Heiko Tietze 2017-10-19 06:21:53 UTC
(In reply to Tamás Zolnai from comment #16)
> So an other option can be to not select any of the buttons...

Makes sense. None in terms of undefined is a good idea.

Unlike Jay I think "None" as a defined state is worse than "Color". No idea what his reasoning is, mine is in comment 15.
Comment 19 Tamás Zolnai 2017-10-22 00:10:16 UTC
Ok, then. So as I see, we can agree in not selecting any of the buttons when we have an ambigous fill. The relevant difference between selecting "None" or none of the buttons is that in the first case transparent color is applied. So I guess Jay also happy with the second case.

@Ekansh Jha: If you are still interested in working on this easy hack, that is the behavior we need to have (selecting none of the buttons on area / background page).
In SvxAreaTabPage::ActivatePage() method you can see the algorithm which selects the button based on fill type. When we have ambigous fill properties you need to skip setting any button. Also make sure that in SvxAreaTabPage::DeactivatePage() method SfxItemSet is not changed, when no button is selected.
To find out how to check whether fill properties are ambigous, I suggest to check what happens on Color page (SvxColorTabPage class). Since it behaves differently in case of ambigouity, I expect this information is there in some form.
Comment 20 Yousuf Philips (jay) (retired) 2017-10-22 18:22:47 UTC
(In reply to Tamás Zolnai from comment #19)
> Ok, then. So as I see, we can agree in not selecting any of the buttons when
> we have an ambigous fill. The relevant difference between selecting "None"
> or none of the buttons is that in the first case transparent color is
> applied. So I guess Jay also happy with the second case.

Fine with any option that doesnt change the fill if the user doesnt set it themselves. I had assume that there wasnt an option to not sent one of the tabs, so that is why i suggested "None".
Comment 21 Ekansh Jha 2017-10-22 18:58:35 UTC
(In reply to Tamás Zolnai from comment #19)
> Ok, then. So as I see, we can agree in not selecting any of the buttons when
> we have an ambigous fill. The relevant difference between selecting "None"
> or none of the buttons is that in the first case transparent color is
> applied. So I guess Jay also happy with the second case.
> 
> @Ekansh Jha: If you are still interested in working on this easy hack, that
> is the behavior we need to have (selecting none of the buttons on area /
> background page).

This discussion confused me a bit.
Please may I get a greater indepth of this issue,

According to me what I understood till now is,
Steps to Reproduce:
1. Open Impress
2. Insert a table
3. Table contect menu -> Table Properties -> Area tab
and then color tab is opening now, do I have to change opening of tab to "none"? 

> In SvxAreaTabPage::ActivatePage() method you can see the algorithm which
> selects the button based on fill type. When we have ambigous fill properties
> you need to skip setting any button. Also make sure that in
> SvxAreaTabPage::DeactivatePage() method SfxItemSet is not changed, when no
> button is selected.

ambiguous property? sorry i didn't get you.

> To find out how to check whether fill properties are ambigous, I suggest to
> check what happens on Color page (SvxColorTabPage class). Since it behaves
> differently in case of ambigouity, I expect this information is there in
> some form.
Comment 22 Tamás Zolnai 2017-10-22 19:06:08 UTC
(In reply to Ekansh Jha from comment #21)
> ambiguous property? sorry i didn't get you.
> 

When you select more cells and the selected cells have different background fill properties. For example one cell has a red background and an other has a green background color. The dialog can display only one color, so in this case it's ambigous what to display (the red or the green color).
Comment 23 Ekansh Jha 2017-10-23 05:51:47 UTC
I couldn't select multiple tabs while opening or in the case of ambigous. I have submitted a patch to my little understanding. I know it there will be alot of mistakes. Please review it.
Comment 24 Heiko Tietze 2017-10-23 07:12:02 UTC
Created attachment 137225 [details]
Table with different fillings

In https://gerrit.libreoffice.org/#/c/43706/ you disable everything, AFAICS. But the idea is to show the color tab when the origin is a solid color and gradient if it's a gradient, etc. This should remain.

Thing is that we have situations where the origin is not clear. For example, table cells with different colors, hatchings, and gradients (as shown in the attachment). If you open the area dialog having the full table selected with the intention to modify just the font, it must not change those backgraound formatting unless the user explicitly starts the action by, e.g. clicking solid color. The suggested solution was to unselect/untoggle all options (aka buttons) because "None" is also a valid type of filling and would clear the table background.

(Until now it's not a big issue since the new dialog is missing in Calc and Writer. Would be another topic if you want to get familiar with this feature.)
Comment 25 Ekansh Jha 2017-10-23 13:24:09 UTC
I have checked the issue, the only problem I am getting is when there is ambigous case(ex. one cell is solid colored while other is gradient). I don't see SvxAreaTabPage::ActivatePage can help me, for not selecting any of the tabs. By default none is getting selected. Please help me to correct it.
Comment 26 Tamás Zolnai 2017-10-23 13:57:15 UTC
(In reply to Ekansh Jha from comment #25)
> I have checked the issue, the only problem I am getting is when there is
> ambigous case(ex. one cell is solid colored while other is gradient). I
> don't see SvxAreaTabPage::ActivatePage can help me, for not selecting any of
> the tabs. By default none is getting selected. Please help me to correct it.

As I suggested earlier, you can check what happens on Color tab (SvxColorTabPage class) in case of ambigous color fills (set red color for one cell and yellow color to an other and select these two cells and open the dialog). To understand what happens you need to read the code. This is part of working on an easy hack. You also can use some debugger to see the code's behavior in runtime: https://wiki.documentfoundation.org/Development/How_to_debug.
Comment 27 Ekansh Jha 2017-10-23 14:10:50 UTC
> As I suggested earlier, you can check what happens on Color tab
> (SvxColorTabPage class) in case of ambigous color fills (set red color for
> one cell and yellow color to an other and select these two cells and open
> the dialog). 
The SvxColorTabPage class is for solid color, when we select two or more cells of solid color but what about patterns and gradients, See attachment by Heiko Tietze, in this table every possible combination is taken into account, and I don't think that this problem can be solved by only SvxColorTabPage class. Please correct me if I am wrong.
Comment 28 Tamás Zolnai 2017-10-23 14:26:58 UTC
(In reply to Ekansh Jha from comment #27)
> > As I suggested earlier, you can check what happens on Color tab
> > (SvxColorTabPage class) in case of ambigous color fills (set red color for
> > one cell and yellow color to an other and select these two cells and open
> > the dialog). 
> The SvxColorTabPage class is for solid color, when we select two or more
> cells of solid color but what about patterns and gradients, See attachment
> by Heiko Tietze, in this table every possible combination is taken into
> account, and I don't think that this problem can be solved by only
> SvxColorTabPage class. Please correct me if I am wrong.

I did not say to solve the problem by SvxColorTabPage, but I suggested that you can check what happens in case of ambigous fill color on that tab page, so you can get an idea how to detect whether the selected cells have ambigous fill. You can also check what happens on background tab page in case of Writer tables (create a table in writer -> add different background color to cells -> select the cells -> open Table Properties -> Background tab)
Comment 29 Ekansh Jha 2017-10-24 03:47:33 UTC
I will try this bug later cause my exams are near. 
Thank you so much for the help.
Comment 30 Xisco Faulí 2018-01-27 03:25:15 UTC Comment hidden (obsolete)
Comment 31 Xisco Faulí 2018-02-27 03:32:09 UTC Comment hidden (obsolete)
Comment 32 Xisco Faulí 2018-03-30 02:32:19 UTC Comment hidden (obsolete)
Comment 33 Xisco Faulí 2018-04-30 02:30:55 UTC Comment hidden (obsolete)
Comment 34 Xisco Faulí 2018-05-31 02:48:38 UTC Comment hidden (obsolete)
Comment 35 Xisco Faulí 2018-06-01 02:14:17 UTC Comment hidden (obsolete)
Comment 36 Xisco Faulí 2018-07-02 02:33:19 UTC Comment hidden (obsolete)
Comment 37 Xisco Faulí 2018-08-02 02:47:47 UTC Comment hidden (obsolete)
Comment 38 Xisco Faulí 2018-09-02 02:48:08 UTC Comment hidden (obsolete)
Comment 39 Tamás Zolnai 2019-04-01 08:29:36 UTC
I close this bug. It's kinda old and was ambigous anyway, so let's just close it.