Closed
Bug 1326189
Opened 7 years ago
Closed 7 years ago
[css-ui] caret-color on visited links
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: rego, Assigned: xidorn)
References
Details
Attachments
(8 files, 7 obsolete files)
494 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
|
Details |
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
Assignee | ||
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → xidorn+moz
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P3
Comment 3•7 years ago
|
||
Jeremy would work on this bug today.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Comment 4•7 years ago
|
||
TODO: maybe add a pref to enlarge the caret width!? MozReview-Commit-ID: KXIMvdTMPPV
Assignee | ||
Comment 5•7 years ago
|
||
That pref already exists, which is ui.caretWidth. See bug 1063162 part 2.
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8826549 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Updated•7 years ago
|
Attachment #8826933 -
Attachment mime type: text/plain → image/png
Comment hidden (obsolete) |
Comment 12•7 years ago
|
||
Attachment #8826933 -
Attachment is obsolete: true
Comment 13•7 years ago
|
||
Updated•7 years ago
|
Attachment #8826935 -
Attachment description: failed test file screenshot 1 (no caret w/ focus) → failed test file screenshot 2 (no caret w/ focus)
Comment 14•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8827037 -
Flags: review?(xidorn+moz) → review?(dholbert)
Assignee | ||
Updated•7 years ago
|
Attachment #8827038 -
Flags: review?(xidorn+moz) → review?(dholbert)
Assignee | ||
Comment 18•7 years ago
|
||
(Hand off both requests to dholbert. The first patch is my patch, which I should not review.)
Comment 19•7 years ago
|
||
(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.
Comment 20•7 years ago
|
||
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.
Comment 21•7 years ago
|
||
(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
Assignee | ||
Comment 22•7 years ago
|
||
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
Comment 23•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
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...:/
Comment 27•7 years ago
|
||
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)
Assignee | ||
Comment 28•7 years ago
|
||
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)
Comment 29•7 years ago
|
||
(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.
Assignee | ||
Comment 30•7 years ago
|
||
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.
Comment 31•7 years ago
|
||
MozReview-Commit-ID: 46DfVPNC46W
Updated•7 years ago
|
Attachment #8826174 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8826932 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8826934 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8826935 -
Attachment is obsolete: true
Comment 32•7 years ago
|
||
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
Comment 33•7 years ago
|
||
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)
Comment 34•7 years ago
|
||
(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.)
Assignee | ||
Comment 35•7 years ago
|
||
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 36•7 years ago
|
||
mozreview-review |
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 37•7 years ago
|
||
mozreview-review |
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 38•7 years ago
|
||
mozreview-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)
Comment 39•7 years ago
|
||
(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 40•7 years ago
|
||
mozreview-review-reply |
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.
Comment 41•7 years ago
|
||
(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.
Assignee | ||
Comment 42•7 years ago
|
||
(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.
Comment 43•7 years ago
|
||
(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.
Updated•7 years ago
|
Attachment #8827926 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 46•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8827037 -
Flags: review?(dholbert)
Updated•7 years ago
|
Attachment #8827038 -
Flags: review?(dholbert)
Assignee | ||
Comment 47•7 years ago
|
||
OK I know the reason... Taking it as I've beeen working on the patch.
Assignee: jeremychen → xidorn+moz
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 53•7 years ago
|
||
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.
Comment 54•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 56•7 years ago
|
||
Hmmm, still failing. Now the test page is empty :/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a4d5384406a03f48eae84d1bdaced62402ef5d4
Comment 57•7 years ago
|
||
(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 58•7 years ago
|
||
mozreview-review |
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 59•7 years ago
|
||
mozreview-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 60•7 years ago
|
||
mozreview-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 61•7 years ago
|
||
mozreview-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 62•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 63•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=afedb4967d7e
Comment 64•7 years ago
|
||
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
Assignee | ||
Comment 65•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14cfe3938c1
Assignee | ||
Comment 66•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ba8000cf29362af3be924c66a2d09997804cbdc
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 73•7 years ago
|
||
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
Comment 74•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2472110ff786 https://hg.mozilla.org/mozilla-central/rev/d3a7a0669c3f https://hg.mozilla.org/mozilla-central/rev/1adf8249feb2 https://hg.mozilla.org/mozilla-central/rev/01d692457554 https://hg.mozilla.org/mozilla-central/rev/282c3b26cf63
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•