Closed Bug 1326189 Opened 6 years ago Closed 6 years ago

[css-ui] caret-color on visited links

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: rego, Assigned: xidorn)

References

Details

Attachments

(8 files, 7 obsolete files)

Example setting caret-color on a visited link
494 bytes, text/html
Details
Bug 1326189 - apply caret-color to visited link.
59 bytes, text/x-review-board-request
Details
Bug 1326189 - (wip) add tests for applying caret-color to visited link.
59 bytes, text/x-review-board-request
Details
Bug 1326189 part 1 - Apply :visited style for caret-color.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1326189 part 2 - Rewrite test_visited_reftest mochitest with async function to make it clearer.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1326189 part 3 - Make test_visited_reftests support needs-focus.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1326189 part 4 - Support caret in test_visited_reftests.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1326189 part 5 - Add reftest for caret-color with :visited.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.28 Safari/537.36

Steps to reproduce:

Open attached file and go with the cursor to the visited link.


Actual results:

caret-color is "white".


Expected results:

caret-color should be "lime".

As you can see in the attached example, "caret-color" cannot be applied to visited links.
If you've a custom style for visited links you might want to change the caret-color for them too.

This causes that Firefox doesn't pass this W3C test:
http://test.csswg.org/source/css-ui-3/caret-color-015.html
Blocks: 1063162
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Assignee: nobody → xidorn+moz
Attached patch WIP patch (obsolete) — — Splinter Review 
This patch itself is enough for fixing this bug, but I don't have time to write test for it...

I think the test should be a reftest in layout/reftests/css-visited.
Unassigning myself from the bug as I don't quite have time working on this at the moment... Anyone interested can pick it up. As I mentioned in comment 1, the only remaining part is just a test...
Assignee: xidorn+moz → nobody
Priority: -- → P3
Jeremy would work on this bug today.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
TODO: maybe add a pref to enlarge the caret width!?

MozReview-Commit-ID: KXIMvdTMPPV
That pref already exists, which is ui.caretWidth. See bug 1063162 part 2.
Comment on attachment 8826549 [details] [diff] [review]
(wip) add tests for applying caret-color to visited link.

In addition to "ui.caretWidth" preference setting, I wonder if I need to disable the caret blinking by setting the hidden preference "ui.caretBlinkTime" manually? Since this test is run in mochitest and I'm not sure if caret blinking is default disabled like reftest does.
Attachment #8826549 - Flags: feedback?(xidorn+moz)
Yeah, you probably should also set ui.caretBlinkTime. I think you can set this pref for the whole mochitest, since it is supposed to run in a way similiar to reftest.

In addition, this test would need focus, which you need to ensure as well. It seems to me that we sometimes need to explicitly wait for focus in mochitests.
Comment on attachment 8826549 [details] [diff] [review]
(wip) add tests for applying caret-color to visited link.

Review of attachment 8826549 [details] [diff] [review]:
-----------------------------------------------------------------

I think having the proper prefs set and ensuring the forcus is set are non-trivial part of this change.

Also in this case, you are comparing caret with caret, which is fragile since you cannot catch the failure that both carets are not displayed. You should either add a negative reference which does not show a caret, or use a static page as the reference. I'd prefer the latter, though.
Attachment #8826549 - Flags: feedback?(xidorn+moz)
Add default pref settings for caret in mochitest.
Add test to test_visited_reftests.html.

FIXME: can't get correct (expected) screenshot from test file.
FIXME: remove debug msgs.

MozReview-Commit-ID: KXIMvdTMPPV
Attachment #8826549 - Attachment is obsolete: true
Attachment #8826933 - Attachment mime type: text/plain → image/png
Attachment #8826935 - Attachment description: failed test file screenshot 1 (no caret w/ focus) → failed test file screenshot 2 (no caret w/ focus)
With the current wip, I can only get a screenshot for the test file, which has no caret shown and the element is not focused either. (see failed test file screenshot 1). If I add a "setTimeout(3000)" to the script right after setting the focus in the test file, I can get a screenshot with the element is focused, but still no caret shown. (see failed test file screenshot 2). 

Need to figure out a way to ensure the caret is shown while taking a screenshot for the test file.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> With the current wip, I can only get a screenshot for the test file, which
> has no caret shown and the element is not focused either. (see failed test
> file screenshot 1). If I add a "setTimeout(3000)" to the script right after
> setting the focus in the test file, I can get a screenshot with the element
> is focused, but still no caret shown. (see failed test file screenshot 2). 
> 
> Need to figure out a way to ensure the caret is shown while taking a
> screenshot for the test file.

Turning on the withCaret flag in snapshotWindow() solves the caret-no-show issue.
Attachment #8827037 - Flags: review?(xidorn+moz) → review?(dholbert)
Attachment #8827038 - Flags: review?(xidorn+moz) → review?(dholbert)
(Hand off both requests to dholbert. The first patch is my patch, which I should not review.)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)
> (Hand off both requests to dholbert. The first patch is my patch, which I
> should not review.)

Thank you for the forwarding.

The added test can pass on my local machine, but it failed on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d29c39020a9 maybe Daniel could provide some feedback about this.
Just pushed another round to try with a patch that enable the logging of failure screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0ca87bb638e

We shall be able to see what happened on try exactly.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #20)
> Just pushed another round to try with a patch that enable the logging of
> failure screenshots:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0ca87bb638e
> 
> We shall be able to see what happened on try exactly.

Looks like the height of the fake caret in reference page is a bit shorter than the height of the caret in test page.
Try to do a  quick fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=add8b539ed2a
The height seems more correct now, but there's still a bit vertical shift between the two screenshots.

Try to fix the shifting issue by clearing outline, border, padding, and margin properties of the fake caret.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9abddab150bb
You can see the reference page of my test [1] for how this can be workaround.

[1] https://hg.mozilla.org/mozilla-central/file/45fa28cafc03/layout/reftests/css-ui/caret-color-01-ref.html
Removing the outer div seems fix the shifting problem on my Linux machine. Let's see how it goes on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=965cfc861c2086e15dd6642b1c038e39bc0dc721
Maybe we still need a outer div to make sure the test page is aligned to the same vertical position as that in the ref page. (see the try failure on Mac in comment 23)

If we keep the div, then the line-height setting causes the shifting thing on different platforms.
If I set "font: 16px/1 Ahem;" for div, there's no shifting issue on Mac, but a slightly shifting down of the caret position in ref page on Linux.
If I set "font: 16px Ahem;" for div, there's no shifting issue on Linux, but a slightly shifting on Mac.

I thought setting font-family to Ahem should have prevented us from this kind of line-height issue...:/
Alright, after I dig a little deeper, I found an issue here.

Example 1:
data:text/html,<span>test</span>

Example 2:
data:text/html,<span style="line-height: 1;>test</span>

On Mac:
If you open example 1, text frame's height is 960 whereas its container's (which should be the <body>) height is 1152 (a default line-height value of roughly 1.2).
If you open example 2, text frame's height and its container's height are both 960, which is expected.

On Linux:
If you open example 1, text frame's height and its container's height are both 1140.
If you open example 2, text frame's height is 1140 whereas its container's height is 960!!! (which is expected to be 1140!?)
There's also a weird shifting for the span element that starts from (0, -90), which might be the cause of making my test fail.
Here's the frame tree dumped by example 2 on Linux: http://pastebin.com/g9UsszzX

Normally, if we would like to test something like example 1, we would sometimes set line-height to 1, to reduce the gap between the texts and its container. (as can be seen from many tests in our code base that set font to "16px/1 Ahem"). However, if we set line-height to 1, an unexpected gap between the text and its line container is shown on Linux; if we don't set line-height to 1, the gap is shown on Mac. So, my test can only passes on one of the two platforms. :-/

Xidorn, does this behavior sounds like a bug to you?
Flags: needinfo?(xidorn+moz)
It doesn't. I'm not surprised that text frame may have different size than its container.

I think, the easiest way now, is probably to make the link an editable block, so that you can simple reuse everything else in my reftest.
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #28)
> It doesn't. I'm not surprised that text frame may have different size than
> its container.
> 
> I think, the easiest way now, is probably to make the link an editable
> block, so that you can simple reuse everything else in my reftest.

I've already make the link editable.
The point is that, if we add text combined with the fake caret <div> in the same line, there's always a shift, either it's on Mac or Linux depending on the line-height setting.
I did tried to add some texts in your reftest, the shift issue would occur as well.
I mean, do something like <a contenteditable style="display: block">, so that it is a block, and you don't need to worry about any issue around inline elements.
MozReview-Commit-ID: 46DfVPNC46W
Attachment #8826174 - Attachment is obsolete: true
Attachment #8826932 - Attachment is obsolete: true
Attachment #8826934 - Attachment is obsolete: true
Attachment #8826935 - Attachment is obsolete: true
Comment on attachment 8827926 [details] [diff] [review]
(wip) add tests for applying caret-color to visited link.

Review of attachment 8827926 [details] [diff] [review]:
-----------------------------------------------------------------

Per discussion with xidorn on IRC, a solution about removing all the texts to eliminate the affection from the inline elements is proposed. Now, the screenshots of the reference file on both Mac and Linux are the same. However, the shift issue is not solved yet.

It seems that in the test file, even we remove all the texts, the contenteditable setting is still implicit that there might be a text child. We need this <a> to be contenteditable so we can test that a caret is indeed shown with the correct color. But, since there's a shifting problem on Linux (see Comment 27), the caret which is supposed to be with the texts is shifted by (0, -90) as well. This makes the caret in the test file is always a bit higher than that in the reference file on Linux. Here's the try failures with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dffaa41e431c3f0104e3c1f2fee8ee6f29d1441a
It seems some of the complexity in getting this test to pass is from the fact that we're trying to make a "fake caret".  This might be overkill -- we already have reftests that compare the caret against a fake-caret (which xidorn added in bug 1063162).

Perhaps it'd be simpler to make *this* reftest just use a real caret in the reference case (with slightly different styling). I'm imagining:
 - testcase is <a href="visited-link"> with a:visited { caret-color: green; }
 - reference case is identical, except with a { caret-color: green; }

This raises the valid question "how can we be sure that the caret is showing up at all" (e.g. maybe focus isn't correctly getting set).  We can address that by adding a "notref" version of the testcase, which is identical to the reference except that it doesn't set focus. And then compare the reference against that "notref" to sanity-check it.

What do you think?
Flags: needinfo?(jeremychen)
(Note that, per the comment in test_visited_reftests.html, the reference case needs to be complete when it's loaded. I suspect we can make that work with a "focus()" call in the reference case's onload handler, though I'm not entirely sure.)
Or alternatively, put a <textarea> inside the <a> so that everything should really be identical to my previous reftest. Notref is something very easy to go wrong unexpectedly.
Comment on attachment 8827037 [details]
Bug 1326189 - apply caret-color to visited link.

https://reviewboard.mozilla.org/r/104872/#review106480

This first part looks good, but you should probably credit xidorn as this part's author in its headers, since it looks like this is exactly the WIP patch that he uploaded at first.

(It makes sense for you to be credited as assignee on the bug since you're closing it out, and as patch-author on part 2 since that's code that you're writing.  But since this "part 1" is already split out, we might as well have its authorship noted correctly.)

I think you can fix this with something like this (after you've hg update'd to this commit):
   hg commit --amend -u "Xidorn Quan <me@upsuper.org>"
or if you use mq:
   hg qref -u "Xidorn Quan <me@upsuper.org>"

Hopefully MozReview will let you push patches where someone else is the author... I don't know for sure if that will work (but if it doesn't, I'd consider that a MozReview bug...)

r- for now for that reason. But r+ once we can get authorship properly noted.  (Worst-case, we can just bypass MozReview for this bug & just use traditional patch files file-attachments)
Attachment #8827037 - Flags: review?(dholbert)
Comment on attachment 8827037 [details]
Bug 1326189 - apply caret-color to visited link.

https://reviewboard.mozilla.org/r/104872/#review106484
Attachment #8827037 - Flags: review-
Comment on attachment 8827038 [details]
Bug 1326189 - (wip) add tests for applying caret-color to visited link.

https://reviewboard.mozilla.org/r/104874/#review106486

Canceling review on tests for now, since it looks like there's a bit more to be done.

(If it's possible to construct a good "fake-caret" version of this test using something like the <textarea> that Xidorn suggests in comment 35, that would be great, since I share his uneasiness about my suggested reference-case-using-a-real-caret strategy possibly going wrong.  The "reference-case-using-a-real-caret strategy" should perhaps be a last resort.)
Attachment #8827038 - Flags: review?(dholbert)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #35)
> Or alternatively, put a <textarea> inside the <a> so that everything should
> really be identical to my previous reftest. Notref is something very easy to
> go wrong unexpectedly.

Add <textarea> inside the <a>, caret position on test file looks aligned with that on reference file, but the height is a bit different on Linux/Windows platform again. Not sure if I add it right or not.... here's the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc3ca0e61ed4fcacb9f59a474ad4b5a34090bbe8
Flags: needinfo?(jeremychen)
Comment on attachment 8827037 [details]
Bug 1326189 - apply caret-color to visited link.

https://reviewboard.mozilla.org/r/104872/#review106480

Oh, sorry for that. Will correct this before sending another round of review.
(In reply to Daniel Holbert [:dholbert] from comment #38)
> Comment on attachment 8827038 [details]
> Bug 1326189 - add tests for applying caret-color to visited link.
> 
> https://reviewboard.mozilla.org/r/104874/#review106486
> 
> Canceling review on tests for now, since it looks like there's a bit more to
> be done.
> 
> (If it's possible to construct a good "fake-caret" version of this test
> using something like the <textarea> that Xidorn suggests in comment 35, that
> would be great, since I share his uneasiness about my suggested
> reference-case-using-a-real-caret strategy possibly going wrong.  The
> "reference-case-using-a-real-caret strategy" should perhaps be a last
> resort.)

Yeah, hope we could eventually find a way to construct a good "fake-caret" version of this test.
Thank you for the feedback.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #39)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #35)
> > Or alternatively, put a <textarea> inside the <a> so that everything should
> > really be identical to my previous reftest. Notref is something very easy to
> > go wrong unexpectedly.
> 
> Add <textarea> inside the <a>, caret position on test file looks aligned
> with that on reference file, but the height is a bit different on
> Linux/Windows platform again. Not sure if I add it right or not.... here's
> the try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fc3ca0e61ed4fcacb9f59a474ad4b5a34090bbe8

Where is the textarea in this test? I don't see any. It seems to be basically the same as the <a contenteditable> one I sent you last night.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #42)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #39)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #35)
> > > Or alternatively, put a <textarea> inside the <a> so that everything should
> > > really be identical to my previous reftest. Notref is something very easy to
> > > go wrong unexpectedly.
> > 
> > Add <textarea> inside the <a>, caret position on test file looks aligned
> > with that on reference file, but the height is a bit different on
> > Linux/Windows platform again. Not sure if I add it right or not.... here's
> > the try:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=fc3ca0e61ed4fcacb9f59a474ad4b5a34090bbe8
> 
> Where is the textarea in this test? I don't see any. It seems to be
> basically the same as the <a contenteditable> one I sent you last night.

You can see the patch on try.
Well, I think maybe it would be better just update the test part WIP on review board. I'm updating it to the current failure now.
Attachment #8827926 - Attachment is obsolete: true
Another idea might be to enlarge the line-height in test file, e.g., something like <a style="font: 16px/5 Ahem;"></a>. And try to make the fake caret in reference file to align to middle, since align-to-middle shall solve the shift issue in theory. I haven't thought through yet (especially the part of how to align the fake caret to middle), but if I still can't make the textarea idea work, maybe the align-to-middle idea is worth a shot.
Attachment #8827037 - Flags: review?(dholbert)
Attachment #8827038 - Flags: review?(dholbert)
OK I know the reason... Taking it as I've beeen working on the patch.
Assignee: jeremychen → xidorn+moz
Oh, I see. So the root cause is that the Ahem font is not loaded before taking a snapshot for the reference file. Lesson learned.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #53)
> Oh, I see. So the root cause is that the Ahem font is not loaded before
> taking a snapshot for the reference file. Lesson learned.

More precisely, font is not loaded properly before applying <span>'s height to fake caret's height, which causes the shift.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #56)
> Hmmm, still failing. Now the test page is empty :/
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5a4d5384406a03f48eae84d1bdaced62402ef5d4

I think you might want to keep the contenteditable setting for <a> in the test file, something like

<a href="visited-page.html" contenteditable><textarea autofocus></textarea></a>

If you remove the contenteditable setting for <a>, when you focus on <textarea>, you'll touch the hyperlink and transfer to visited-page.html, which is an empty page. That's the reason why you got the screenshot of the test page empty.
Comment on attachment 8828659 [details]
Bug 1326189 part 1 - Apply :visited style for caret-color.

https://reviewboard.mozilla.org/r/106006/#review107158
Attachment #8828659 - Flags: review?(dholbert) → review+
Comment on attachment 8828660 [details]
Bug 1326189 part 2 - Rewrite test_visited_reftest mochitest with async function to make it clearer.

https://reviewboard.mozilla.org/r/106008/#review107160

Commit message nit:
> Rewrite test_visited_reftest mochitest

Please add an "s" to the end of "reftest", to more closely match this test's filename (which is test_visited_reftests.html, plural)

::: layout/style/test/test_visited_reftests.html:130
(Diff revision 1)
> -  var testData =
> -    { op: splitData[0], test: splitData[1], reference: splitData[2] };
> -  var tries = 0;
> -
> -  // Maintain state specific to this test in the closure exposed to all
> -  // the functions nested inside this one.
> -
> -  function startIframe(url)
> -  {
> -    var element = document.createElement("iframe");
> +  let testData = {
> +    equal: splitData[0] == "==",
> +    test: splitData[1],
> +    reference: splitData[2],
> +  };
> +
> +  let test = startIframe(testData.test);
> +  let reference = startIframe(testData.reference);
> +  let referenceSnapshot = snapshotWindow(await reference);
> +  let testWindow = await test;

The "test" and "reference" fields & variables here could stand to have more specific names, to make it a little easier to distinguish between the field & the local-variable, & to help reason about what they mean.

I'd suggest:
 - Rename the testData {} fields to "testFile" and "referenceFile" (or "refFile"), to make it clearer that those are filenames.
 - Rename the local variables (which store promises for windows) to "testWinPromise" and "refWinPromise".

...or something along those lines.
Attachment #8828660 - Flags: review?(dholbert) → review+
Comment on attachment 8828661 [details]
Bug 1326189 part 3 - Make test_visited_reftests support needs-focus.

https://reviewboard.mozilla.org/r/106010/#review107228

::: layout/style/test/test_visited_reftests.html:131
(Diff revision 1)
> -    equal: splitData[0] == "==",
> -    test: splitData[1],
> -    reference: splitData[2],
> +  while (true) {
> +    let op = splitData.shift();
> +    if (op == "needs-focus") {

Could you check that splitData is non-empty before you shift() it?

And if it is empty, fail an ok(false, "...") check and return.

Without that fix, it looks like a typo in test-data (particularly a missing "!="/"==") could make this loop run forever, since we'd never enter the clause with the break statement, and since shift() seems to happily return 'undefined' when called on an empty array.  And the error output for that sort of typo would probably be arcane (test timed out) and hard to debug. So, might as well foolproof the loop against that, to help us down the line. :)
Attachment #8828661 - Flags: review?(dholbert) → review+
Comment on attachment 8828662 [details]
Bug 1326189 part 4 - Support caret in test_visited_reftests.

https://reviewboard.mozilla.org/r/106014/#review107230

::: layout/style/test/test_visited_reftests.html:113
(Diff revision 1)
>  }
>  
>  async function runTests() {
>    SimpleTest.waitForExplicitFinish();
>    SimpleTest.requestFlakyTimeout("async link coloring");
> +  await SpecialPowers.pushPrefEnv({'set': [['ui.caretWidth', 16]]});

This is arcane enough to probably merit a code-comment... Perhaps something like:

  // Set caret to a known size, for tests of :visited caret-color styling:
Attachment #8828662 - Flags: review?(dholbert) → review+
Comment on attachment 8828663 [details]
Bug 1326189 part 5 - Add reftest for caret-color with :visited.

https://reviewboard.mozilla.org/r/106012/#review107232

This seems fine, if it passes (though it sounds like it doesn't quite yet).

Feel free to carry forward r+ if remaining changes are minor.
Attachment #8828663 - Flags: review?(dholbert) → review+
FWIW, here's the positive try result on all Linux/Mac/Windows platforms as I suggested in comment 57.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c503af17192b2b753e05958c35d012f52e7a242e
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2472110ff786
part 1 - Apply :visited style for caret-color. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d3a7a0669c3f
part 2 - Rewrite test_visited_reftest mochitest with async function to make it clearer. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1adf8249feb2
part 3 - Make test_visited_reftests support needs-focus. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/01d692457554
part 4 - Support caret in test_visited_reftests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/282c3b26cf63
part 5 - Add reftest for caret-color with :visited. r=dholbert
You need to log in before you can comment on or make changes to this bug.