Defect - during keyboard showing/hiding, sometimes hidden appbars show up

RESOLVED FIXED in Firefox 25

Status

Firefox for Metro
App Bar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwilde, Assigned: jwilde)

Tracking

unspecified
Firefox 25
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [preview])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Steps to reproduce:
- Perform a series of actions that open and close the keyboard

Expected behavior:
- Keyboard goes down, shouldn't see any bars that haven't already been shown.

Actual behavior:
- We see remnants of hidden toolbars when the keyboard closes sometimes.

Not a breaking issue, but it looks sloppy and makes the browser feel a bit broken.
(Assignee)

Comment 1

5 years ago
p=1
Status: NEW → ASSIGNED
Whiteboard: p=1
(Assignee)

Comment 2

5 years ago
Created attachment 783329 [details] [diff] [review]
patch v1
(Assignee)

Updated

5 years ago
Attachment #783329 - Flags: review?(mbrubeck)
Won't this patch remove the slide out transition of appbars?
(Assignee)

Comment 4

5 years ago
(In reply to Rodrigo Silveira [:rsilveira] from comment #3)
> Won't this patch remove the slide out transition of appbars?

Good catch.
(Assignee)

Updated

5 years ago
Attachment #783329 - Flags: review?(mbrubeck)
(Assignee)

Comment 5

5 years ago
Created attachment 784157 [details] [diff] [review]
fixupappbars-v0.1
Attachment #783329 - Attachment is obsolete: true
Attachment #784157 - Flags: review?(mbrubeck)
(Assignee)

Comment 6

5 years ago
Created attachment 784286 [details] [diff] [review]
fixupappbars-v0.2

Missed folding in a chunk of the patch.
Attachment #784157 - Attachment is obsolete: true
Attachment #784157 - Flags: review?(mbrubeck)
Attachment #784286 - Flags: review?(mbrubeck)
(Assignee)

Comment 7

5 years ago
Created attachment 784288 [details] [diff] [review]
fixupappbars-v0.3

Sorry for the bugspam.
Attachment #784286 - Attachment is obsolete: true
Attachment #784286 - Flags: review?(mbrubeck)
Attachment #784288 - Flags: review?(mbrubeck)
(Assignee)

Comment 8

5 years ago
Created attachment 784305 [details] [diff] [review]
fixupappbars-v0.4

Missed a case where the navbar wouldn't be shown on the start page.
Attachment #784288 - Attachment is obsolete: true
Attachment #784288 - Flags: review?(mbrubeck)
Attachment #784305 - Flags: review?(mbrubeck)
Comment on attachment 784157 [details] [diff] [review]
fixupappbars-v0.1

Drive by comment, for "use strict" compatibility, we should name the transitionend handler function rather than using arguments.callee. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_and_function_scope/Strict_mode
Attachment #784157 - Flags: feedback+

Updated

5 years ago
Whiteboard: p=1
Comment on attachment 784305 [details] [diff] [review]
fixupappbars-v0.4

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

r=mbrubeck with one nit:

::: browser/metro/base/content/bindings/appbar.xml
@@ +38,5 @@
>  
> +            let self = this;
> +            this.setAttribute("hiding", "true");
> +            this.addEventListener("transitionend", function () {
> +              self.removeEventListener("transitionend", arguments.callee, false);

Please give the function a name instead of using arguments.callee.  (arguments.callee prevents certain optimizations and is forbidden in strict mode.  Neither is a real concern here at the moment but I want to avoid it everywhere for consistency and habit's sake.)

::: browser/metro/base/content/helperui/FindHelperUI.js
@@ +122,5 @@
>        findbar.show();
> +
> +      this.search(this._textbox.value);
> +      this._textbox.select();
> +      this._textbox.focus();

If you haven't already, please make sure this doesn't require any test changes.
Attachment #784305 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 11

5 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> Comment on attachment 784305 [details] [diff] [review]
> fixupappbars-v0.4
> 
> Review of attachment 784305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=mbrubeck with one nit:
> 
> ::: browser/metro/base/content/bindings/appbar.xml
> @@ +38,5 @@
> >  
> > +            let self = this;
> > +            this.setAttribute("hiding", "true");
> > +            this.addEventListener("transitionend", function () {
> > +              self.removeEventListener("transitionend", arguments.callee, false);
> 
> Please give the function a name instead of using arguments.callee. 
> (arguments.callee prevents certain optimizations and is forbidden in strict
> mode.  Neither is a real concern here at the moment but I want to avoid it
> everywhere for consistency and habit's sake.)

Makes sense. Will do.

> ::: browser/metro/base/content/helperui/FindHelperUI.js
> @@ +122,5 @@
> >        findbar.show();
> > +
> > +      this.search(this._textbox.value);
> > +      this._textbox.select();
> > +      this._textbox.focus();
> 
> If you haven't already, please make sure this doesn't require any test
> changes.

This specific change was actually made to avoid test changes. :D

When the findbar is visiblity: hidden, the textbox can't accept focus (which broke a test), so I had to move that until after the visibility was reset.
Whiteboard: [preview]
https://hg.mozilla.org/mozilla-central/rev/42c35a94058b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Please land on fx-team now (see newsgroup post) - this unfortunately caused conflicts with metro landings on fx-team. Thank you :-)
Depends on: 901094
You need to log in before you can comment on or make changes to this bug.