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

Status: ASSIGNED

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

Description

[Dão Gottwald [:dao]]

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

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1378313 - Fix up bogus code showing the reload button as enabled when it's disabled.

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

Review commit: https://reviewboard.mozilla.org/r/154436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154436/

Comment 2

[Johann Hofmann [:johannh]]

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.

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff0abac31f47
Fix up bogus code showing the reload button as enabled when it's disabled. r=johannh

Comment 5

[Johann Hofmann [: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!

Comment 6

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=112197596&repo=autoland

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6b6ba8101c9
Backed out changeset ff0abac31f47 for causing browser_formless_submit_chrome.js | Test timed out

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8883514 [details]
Bug 1378313 - Fix up bogus code showing the reload button as enabled when it's disabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154436/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f49a21b8b4bd
Fix up bogus code showing the reload button as enabled when it's disabled. r=johannh

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8883514 [details]
Bug 1378313 - Fix up bogus code showing the reload button as enabled when it's disabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154436/diff/2-3/

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/784a407bede3
Backed out changeset f49a21b8b4bd for perma failurs like browser_temporary_permissions_navigation.js | Test timed out

Comment 13

[Dão Gottwald [:dao]]

<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

Comment 14

[Dão Gottwald [:dao]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff995f2f71f2
Fix up bogus code showing the reload button as enabled when it's disabled. r=johannh

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 17

[Pulsebot]

Backout by [e-mail]:
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

Comment 18

[Phil Ringnalda (:philor)]

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.

Comment 19

[Joe Hildebrand [:jhildebrand] (UTC-6)]

Moving this from Firefox:General and marking it 57wontfix.  NI me if you disagree.

Comment 20

[:Gijs (he/him)]

Dão, do you think this is important enough to track for 58? Are you still working on this?

Comment 21

[Dão Gottwald [:dao]]

(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.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:Gijs, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 23

[:Gijs (he/him)]

The bot is just wrong.