Bug 102037 – [EFL] Refactor theme to choose whether to support foreground color of selection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

WebKit/Efl provides a way to change the foreground color of selection.
But, we did not support keeping the text color although text was selected.

Patch will be added.

[Ryuan Choi]

Created attachment 173800 [details]
Patch

[Jinwoo Song]

Comment on attachment 173800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173800&action=review

> Source/WebCore/ChangeLog:10
> +        which isselected.

s/isselected/is selected/

[Ryuan Choi]

Created attachment 173810 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 173810 [details]
Patch

It looks this patch may influence on pixel-test. Did you check this ?

[Ryuan Choi]

(In reply to comment #4)
> (From update of attachment 173810 [details])
> It looks this patch may influence on pixel-test. Did you check this ?

I will check, but it should not change anything.
Default theme provides same foreground color.

[Laszlo Gombos]

Comment on attachment 173810 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review

Any idea how to test this code new code path ?

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

You should remove this line.

[Gyuyoung Kim]

Comment on attachment 173810 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Remove this comment.

> Source/WebCore/platform/efl/RenderThemeEfl.h:109
> +    virtual bool supportsSelectionForegroundColors() const;

I prefer to put same functions in a place. So, it looks this can be moved to 'bool supportsControlTints()' below.

virtual bool supportsControlTints() const { return true; }

[Ryuan Choi]

Created attachment 173839 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 173810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review
> 
> Any idea how to test this code new code path ?
> 
> > Source/WebCore/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> You should remove this line.

Oops it's mistake. removed.(In reply to comment #7)
> (From update of attachment 173810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> Remove this comment.
Done, sorry.

> 
> > Source/WebCore/platform/efl/RenderThemeEfl.h:109
> > +    virtual bool supportsSelectionForegroundColors() const;
> 
> I prefer to put same functions in a place. So, it looks this can be moved to 'bool supportsControlTints()' below.
> 
> virtual bool supportsControlTints() const { return true; }

I tried to keep the similar position with base class.
But I can move this if you want.

[Gyuyoung Kim]

(In reply to comment #9)

> I tried to keep the similar position with base class.
> But I can move this if you want.

Yes, I prefer it.

[Ryuan Choi]

Created attachment 173854 [details]
Patch

[Ryuan Choi]

Comment on attachment 173854 [details]
Patch

Thank you kangil.

[WebKit Review Bot]

Comment on attachment 173854 [details]
Patch

Clearing flags on attachment: 173854

Committed r134694: <http://trac.webkit.org/changeset/134694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

The API test ewk_test2_view is asserting on _ASSERT_ON_RELEASE_RETURN_VAL(ok, false, "Could not get color class '%s'\n", colorClass);

Do we need to change the test after this patch?

[WebKit Review Bot]

Re-opened since this is blocked by bug 102481

[Ryuan Choi]

(In reply to comment #15)
> The API test ewk_test2_view is asserting on _ASSERT_ON_RELEASE_RETURN_VAL(ok, false, "Could not get color class '%s'\n", colorClass);
> 
> Do we need to change the test after this patch?

I missed to change big_button.edj not to have unnecessary warning.

But I think that it would be just additional warning, not fail case of API test.

Anyway, I will make patch including bug_button.edj

[Ryuan Choi]

Created attachment 174953 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Laszlo's question in comment #6 has not been answered yet -- any idea how to test this? It's still not clear to me from the ChangeLog what this is actually fixing.

[Ryuan Choi]

(In reply to comment #19)
> Laszlo's question in comment #6 has not been answered yet -- any idea how to test this? It's still not clear to me from the ChangeLog what this is actually fixing.

Sorry, I didn't catch it.

For test,
I don't have idea now.

IMHO, to tests this,
we make pixel tests for API tests (or any framework to test theme file).

It's because this changes just affect painting of selected paragraph.

WebCore choose foreground color of selection if it was given.
If not, it will paint default color of selected text.

But, we can not have a way not to give foreground color to WebCore.
So, this patch adds a new feature to disable foreground color to paint default color of selected text.

I will consider to update change log.

Thanks.

[Ryuan Choi]

Created attachment 175229 [details]
Patch

[Ryuan Choi]

Created attachment 176455 [details]
Patch

[Chris Dumez]

Comment on attachment 176455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176455&action=review

> Source/WebCore/ChangeLog:12
> +        However, EFL port does not have a way to disable supports of foreground

"support"

> Source/WebCore/ChangeLog:14
> +        active and inactive to foreground and background in order to disable supports

ditto

> Source/WebKit2/ChangeLog:8
> +        * UIProcess/API/efl/tests/resources/big_button_theme.edj: Recreated.

We really should not have a binary in our tree. Why don't you upload the edc instead and compile it with the others?
At the moment, you are the only one who can update this file.

[Ryuan Choi]

Created attachment 176464 [details]
Patch

[Ryuan Choi]

(In reply to comment #23)
> (From update of attachment 176455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176455&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        However, EFL port does not have a way to disable supports of foreground
> 
> "support"
> 
> > Source/WebCore/ChangeLog:14
> > +        active and inactive to foreground and background in order to disable supports
> 
> ditto
> 
Thank you, I fixed.

> > Source/WebKit2/ChangeLog:8
> > +        * UIProcess/API/efl/tests/resources/big_button_theme.edj: Recreated.
> 
> We really should not have a binary in our tree. Why don't you upload the edc instead and compile it with the others?
> At the moment, you are the only one who can update this file.

Good point. I agree with you.
I created Bug 103511 for it and removed changes from this bug because it does not make regression and print additional warning anymore.

[WebKit Review Bot]

Comment on attachment 176464 [details]
Patch

Clearing flags on attachment: 176464

Committed r136005: <http://trac.webkit.org/changeset/136005>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.