Issue 5817 - Retry sending suggested filters to block element dialog

Issue 5817 - Retry sending suggested filters to block element dialog

[kzar, 2017-11-21 15:39:20]

Patch Set 1

I'm not happy about this hack, but as it stands the "block element" feature is
completely broken for our Windows users. I'm open to suggestions if there's a
better approach.

https://codereview.adblockplus.org/29613777/diff/29613778/composer.html
File composer.html (right):

https://codereview.adblockplus.org/29613777/diff/29613778/composer.html#newco...
composer.html:93: <textarea id="filters" class="i18n_loading"
disabled></textarea>
Since the user has to wait for 1 second it looks much nicer disabling the field
and displaying a "Loading..." message. Otherwise the user wouldn't know what was
going on, and also might start typing a filter which would then be overwritten.

https://codereview.adblockplus.org/29613777/diff/29613778/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29613777/diff/29613778/lib/filterComposer....
lib/filterComposer.js:246: setTimeout(doInit, 1000);
Initially I retried every 100ms, but then I found the message was sent seemingly
successfully yet never received. Adding a 1 second delay seemed to work, but of
course this will probably vary depending on the computer.

[Wladimir Palant, 2017-11-23 11:49:00]

https://codereview.adblockplus.org/29613777/diff/29613778/composer.html
File composer.html (right):

https://codereview.adblockplus.org/29613777/diff/29613778/composer.html#newco...
composer.html:96: <button id="addButton" class="i18n_add"></button>
Shouldn't this button be disabled as well?

https://codereview.adblockplus.org/29613777/diff/29613778/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29613777/diff/29613778/lib/filterComposer....
lib/filterComposer.js:242: // Firefox 57 sometimes sets the status for a window
to "complete"
Nit: Please drop the version number, we don't know which Firefox versions are
affected.

[kzar, 2017-11-23 12:31:54]

Patch Set 2 : Rebased, addressed Wladimir's feedback

https://codereview.adblockplus.org/29613777/diff/29613778/composer.html
File composer.html (right):

https://codereview.adblockplus.org/29613777/diff/29613778/composer.html#newco...
composer.html:96: <button id="addButton" class="i18n_add"></button>
On 2017/11/23 11:49:00, Wladimir Palant wrote:
> Shouldn't this button be disabled as well?

Good point, Done.

https://codereview.adblockplus.org/29613777/diff/29613778/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29613777/diff/29613778/lib/filterComposer....
lib/filterComposer.js:242: // Firefox 57 sometimes sets the status for a window
to "complete"
On 2017/11/23 11:49:00, Wladimir Palant wrote:
> Nit: Please drop the version number, we don't know which Firefox versions are
> affected.

Done.

https://codereview.adblockplus.org/29613777/diff/29616569/composer.js
File composer.js (right):

https://codereview.adblockplus.org/29613777/diff/29616569/composer.js#newcode102
composer.js:102: $("#addButton").button("option", "disabled", false);
I'm unhappy to add new code that uses jQuery however I noticed that jQuery UI
added a bunch of special classes to the element and simply setting the disabled
attribute to false wasn't enough to get the button working again.

[Wladimir Palant, 2017-11-23 13:50:31]

LGTM

[Thomas Greiner, 2017-11-23 13:59:36]

LGTM