879631 - Change - Remove form navigation code

Status: VERIFIED FIXED

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

Description

[Rodrigo Silveira [:rsilveira] [:rodms]]

Quoting mbrubeck from comment 4 on bug 879115:

"At some point we should simplify the content-navigator code.  It used to show not only the findbar but also the "form navigator" (long since removed).  We don't need all that functionality anymore."

Comment 1

[Jonathan Wilde [:jwilde]]

Attachment: patch v1

Drops the formhelper components from the application.

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 761155 [details] [diff] [review]
patch v1

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

This is too far-reaching.  Sorry, I should have been more specific in my comment.  We do use the parts of FormHelper that handle autocompletion, select menus, and scrolling to show selected elements.

The main thing that is unused and should be removed is base/content/bindings/bindings.xml#content-navigator - we used to use this binding to make the "navigator" bar change behavior depending on whether it was in "find in page" mode or "form helper" mode.  Now we have only find-in-page, so we can ditch this binding and just put the toolbar content directly into browser.xul.

::: browser/metro/base/content/browser.xul
@@ -114,5 @@
>  
> -    <!-- forms navigation -->
> -    <command id="cmd_formPrevious" oncommand="FormHelperUI.goToPrevious();"/>
> -    <command id="cmd_formNext" oncommand="FormHelperUI.goToNext();"/>
> -    <command id="cmd_formClose" oncommand="FormHelperUI.hide();"/>

Yeah, this stuff is dead code and should be removed.

::: browser/metro/base/content/helperui/SelectHelperUI.js
@@ -30,5 @@
>    show: function selectHelperShow(aList, aTitle, aRect) {
> -    if (AnimatedZoom.isZooming()) {
> -      FormHelperUI._waitForZoom(this.show.bind(this, aList, aTitle, aRect));
> -      return;
> -    }

Yes, this can be removed too.

Comment 3

[Marco Mucci [:MarcoM]]

Hi Jonathan, I can meet with you on Thursday when I return from vacation.  In the meantime, I'll need a point estimation for this Change Story you have selected for Iteration #9.  I've copied Brian Bondy to give you a brief on how our point estimations works.

Comment 4

[Brian R. Bondy [:bbondy]]

So points are basic estimations for level of effort. If you can do something in a day I usually just put 1 point.

Possible point values are basically Fibonacci numbers, i.e. 1,2,3,5,8,13. If you are going beyond that, we probably want to split up the bug instead into multiple parts.   

If you go look in the Whiteboard of any Metro bug, you'll see this included in the text: p=SomeNumber.  SomeNumber is the number of points assigned.  If no value is assigned yet you'll see p=0.  Sometimes the header is missing and you can just CC Marco or a QA member to help you fill it out.

If a bug is in the current iteration and SomeNumber is set to 0, feel free to put your point estimation in.  If you want to provide a point estimation for any bug not in the current iteration, just put a new comment with p=WhateverYouThink.  When the iteration starts the comments with p=WhateverYouThink will usually be moved up into the Whiteboard header.

Note there is a distinction between:
p=1 and p1.  The former is a point value, the later is the importance level of the bug. We use both so just note if there's an '=' then we're talking about points and if there is no '=' then we're talking about importance.  The importance levels are set in the Importance Bugzilla header instead of the Whiteboard Bugzilla header. P1 usually means fix next iteration, P2 means fix before release but maybe not next iteration. P3 means we probably want to fix before release but we'll re-evaluate. P4 or higher means it's a v2 item.

Comment 5

[Jonathan Wilde [:jwilde]]

Ah, okay! All of that seems to make sense.

Marco, would be glad to meet with you on Thursday.
Brian, thanks for the overview!

This really should be a p=1 bug, but I'm running a bit slow since I'm still getting a few dev environment things spun up.

Comment 6

[Brian R. Bondy [:bbondy]]

Ya p=1 is fine, points are not really about time at all, just estimated level of effort that one would spend fixing it.

Comment 7

[Jonathan Wilde [:jwilde]]

Attachment: patch v2

Replaces use of content-navigator xbl with appbar xbl, removes various bit of older code, and fixes all of the alignment issues.

Comment 8

[Jonathan Wilde [:jwilde]]

Attachment: patch v2.1

Comment 9

[Jonathan Wilde [:jwilde]]

New patch was posted because the old one added spaces on blank lines.

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 761593 [details] [diff] [review]
patch v2.1

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

Thanks!

Comment 11

[Jonathan Wilde [:jwilde]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65cc101ff835

Comment 12

[Marco Mucci [:MarcoM]]

Hey Brian.  Thanks very much for providing the detail explanation.  Your points in Comment #4 and #6 were perfect.

Hi Jonathan, thanks very much for providing the point estimate.  I'll send you a meeting request for Thursday to meet.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/65cc101ff835

Comment 14

[juan becerra [:juanb]]

Tested for iteration #9 on 2013-07-10 using latest nightly. I'm not sure how to test this, so I tried making sure the functionality in the find bar and form filling was working as expected, which it is for the most part.

I'm filling defect bugs for issues that I saw like bug 892224 but which may not be related to this.

Comment 15

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.I tried making sure the functionality in the find bar and form filling was working as expected, which it is for the most part.