Closed Bug 899726 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox for Metro Graveyard :: App Bar, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: jwilde, Assigned: jwilde)

References

Details

(Whiteboard: [preview])

Attachments

(1 file, 4 obsolete files)

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.
p=1
Status: NEW → ASSIGNED
Whiteboard: p=1
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #783329 - Flags: review?(mbrubeck)
Won't this patch remove the slide out transition of appbars?
(In reply to Rodrigo Silveira [:rsilveira] from comment #3)
> Won't this patch remove the slide out transition of appbars?

Good catch.
Attachment #783329 - Flags: review?(mbrubeck)
Attached patch fixupappbars-v0.1 (obsolete) — Splinter Review
Attachment #783329 - Attachment is obsolete: true
Attachment #784157 - Flags: review?(mbrubeck)
Attached patch fixupappbars-v0.2 (obsolete) — Splinter Review
Missed folding in a chunk of the patch.
Attachment #784157 - Attachment is obsolete: true
Attachment #784157 - Flags: review?(mbrubeck)
Attachment #784286 - Flags: review?(mbrubeck)
Attached patch fixupappbars-v0.3 (obsolete) — Splinter Review
Sorry for the bugspam.
Attachment #784286 - Attachment is obsolete: true
Attachment #784286 - Flags: review?(mbrubeck)
Attachment #784288 - Flags: review?(mbrubeck)
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+
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+
(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
Closed: 11 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.

Attachment

General

Created:
Updated:
Size: