Open Bug 1378313 Opened 5 years ago Updated 4 years ago

Fix up bogus code showing the reload button as enabled when it's disabled

Categories

(Firefox :: Toolbars and Customization, defect, P2)

defect

Tracking

()

ASSIGNED
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

Bug 1376893's approach is wrong in various ways:

- It shows the reload button as enabled for about:blank

- The disabled state styling is implemented in browser/themes/shared/toolbarbuttons.inc.css, and could be implemented differently. browser/base/content/browser.css should not depend on theme specifics.

- :not(:-moz-window-inactive) only begins to make sense on Mac, and obviously prevents the fix from taking effect in inactive windows
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment on attachment 8883514 [details]
Bug 1378313 - Fix up bogus code showing the reload button as enabled when it's disabled.

https://reviewboard.mozilla.org/r/154436/#review159740

Ok, this should work better. Thanks!

The only thing that's a bit annoying is that pointer-events: none makes the hover background appear 650ms later, but I guess it shouldn't be a big problem in practice.
Attachment #8883514 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #2)
> The only thing that's a bit annoying is that pointer-events: none makes the
> hover background appear 650ms later, but I guess it shouldn't be a big
> problem in practice.

[disabled] does the same.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff0abac31f47
Fix up bogus code showing the reload button as enabled when it's disabled. r=johannh
(In reply to Dão Gottwald [::dao] from comment #3)
> (In reply to Johann Hofmann [:johannh] from comment #2)
> > The only thing that's a bit annoying is that pointer-events: none makes the
> > hover background appear 650ms later, but I guess it shouldn't be a big
> > problem in practice.
> 
> [disabled] does the same.

Indeed!
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=112197596&repo=autoland
Flags: needinfo?(dao+bmo)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6b6ba8101c9
Backed out changeset ff0abac31f47 for causing browser_formless_submit_chrome.js | Test timed out
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f49a21b8b4bd
Fix up bogus code showing the reload button as enabled when it's disabled. r=johannh
backed out again, seems this makes https://treeherder.mozilla.org/logviewer.html#?job_id=112216924&repo=autoland perma failure - also since this bounced twice we need a try run here
Flags: needinfo?(dao+bmo)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/784a407bede3
Backed out changeset f49a21b8b4bd for perma failurs like browser_temporary_permissions_navigation.js | Test timed out
<dao> damn intermittents making it harder to spot permanent failures
<Tomcat|sheriffduty> yeah
<Tomcat|sheriffduty> dao i filed bug 1364373 to make this better
<dao> Tomcat|sheriffduty: can you spot any other issue with my push? otherwise I'll reland with that test fixed
<dao> Tomcat|sheriffduty: I don't intend to do a try run. the problem here is identifying failures actually caused by my patch, and if I can't figure that out on https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ff0abac31f47f58e53f13c6cad84f6a5c74cefca and https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f49a21b8b4bd5f97d505bd95d4a11982df3f6f82 then I won't see it on try either
Flags: needinfo?(dao+bmo)
I didn't get a response from Tomcat but did a quick Linux-only artifact try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3876db696eeca00591432b954ededbe4476a0b29
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff995f2f71f2
Fix up bogus code showing the reload button as enabled when it's disabled. r=johannh
Backed out for frequently failing test_accessiblecaret_cursor_mode.py on Linux opt:

https://hg.mozilla.org/integration/autoland/rev/8d3213b771bda968d122e0c5544470c1b4f60f02

Pushes with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=b16e500c4f88832546b6bc659edd304831ec0cf3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&group_state=expanded&filter-searchStr=linux%20opt%20tc-e10s%28Mn%29&tochange=8d3213b771bda968d122e0c5544470c1b4f60f02
Failure log (failures at different subtests): https://treeherder.mozilla.org/logviewer.html#?job_id=112276641&repo=autoland

[task 2017-07-06T15:27:24.608252Z] 15:27:24     INFO -  1499354844576	Marionette	TRACE	37 <- [1,65,null,{"value":"<html id=\"html\"><head>\n    <title>Marionette tests for AccessibleCaret in cursor mode</title>\n    <style>\n    .block {\n      width: 10em;\n      height: 6em;\n      word-wrap: break-word;\n      overflow: auto;\n    }\n    </style>\n  </head>\n  <body>\n    <div>\n      <input id=\"input\" value=\"ABCDEFGHI\">\n      <input id=\"input-padding\" style=\"padding: 1em;\" value=\"ABCDEFGHI\">\n    </div>\n    <br>\n    <div>\n      <textarea name=\"textarea\" id=\"textarea\" rows=\"4\" cols=\"6\">ABCDEFGHI</textarea>\n      <textarea id=\"textarea-one-line\" rows=\"3\">ABCDEFGHI</textarea>\n    </div>\n    <br>\n    <div class=\"block\" id=\"contenteditable\" contenteditable=\"true\"><br><br>ABCDEFGHI!</div>\n  \n\n</body></html>"}]
[task 2017-07-06T15:27:24.617582Z] 15:27:24     INFO - TEST-UNEXPECTED-FAIL | test_accessiblecaret_cursor_mode.py AccessibleCaretCursorModeTestCase.test_move_cursor_to_front_by_dragging_caret_to_front_br_element | AssertionError: u'!\n\nABCDEFGHI' != u'ABCDEFGHI!'
[task 2017-07-06T15:27:24.618081Z] 15:27:24     INFO - - !
[task 2017-07-06T15:27:24.618907Z] 15:27:24     INFO - - 
[task 2017-07-06T15:27:24.619488Z] 15:27:24     INFO - - ABCDEFGHI+ ABCDEFGHI!?          +
[task 2017-07-06T15:27:24.620036Z] 15:27:24     INFO - Traceback (most recent call last):
[task 2017-07-06T15:27:24.620300Z] 15:27:24     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 156, in run
[task 2017-07-06T15:27:24.621040Z] 15:27:24     INFO -     testMethod()
[task 2017-07-06T15:27:24.621700Z] 15:27:24     INFO -   File "/home/worker/workspace/build/tests/marionette/tests/layout/base/tests/marionette/test_accessiblecaret_cursor_mode.py", line 262, in test_move_cursor_to_front_by_dragging_caret_to_front_br_element
[task 2017-07-06T15:27:24.622425Z] 15:27:24     INFO -     self.assertEqual(target_content, sel.content)
Flags: needinfo?(dao+bmo)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/adba81ed9362
Backed out changeset ff995f2f71f2 for frequently failing test_accessiblecaret_cursor_mode.py on Linux opt. r=backout
Also very frequent "test_restyles.html | Animations running on the compositor should not update style on the main thread - got 1, expected +0" like https://treeherder.mozilla.org/logviewer.html#?job_id=112247081&repo=autoland and "test_restyles.html | CSS transitions running on the compositor should not update style on the main thread - got 1, expected +0" like https://treeherder.mozilla.org/logviewer.html#?job_id=112264637&repo=autoland which you will have to take responsibility for ensuring are fixed, because apparently our sheriffs believe that words don't mean things, and there's no difference between "should not update style on the main thread" and "should only update style once after finish() is called" so they'll just misstar them as existing bugs about something entirely different.
Moving this from Firefox:General and marking it 57wontfix.  NI me if you disagree.
Component: General → Toolbars and Customization
Priority: -- → P2
Flags: needinfo?(dao+bmo)
Dão, do you think this is important enough to track for 58? Are you still working on this?
Flags: needinfo?(dao+bmo)
(In reply to :Gijs (slow, PTO recovery mode) from comment #20)
> Dão, do you think this is important enough to track for 58?

No.

> Are you still working on this?

Not right now but I intend to pick this up again when I find the time.
Flags: needinfo?(dao+bmo)
See Also: → 1421463
You need to log in before you can comment on or make changes to this bug.