Closed Bug 879631 Opened 7 years ago Closed 7 years ago

Change - Remove form navigation code

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: rsilveira, Assigned: jwilde)

References

Details

(Whiteboard: [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1 status=verified)

Attachments

(1 file, 2 obsolete files)

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."
Summary: Remove form navigation code → Change - Remove form navigation code
Whiteboard: [shovel-ready] → [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=0
Assignee: nobody → hello
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
Drops the formhelper components from the application.
Attachment #761155 - Attachment is patch: true
Attachment #761155 - Attachment mime type: text/x-patch → text/plain
Attachment #761155 - Flags: review?(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.
Attachment #761155 - Flags: review?(mbrubeck) → review-
Blocks: metrov1it9
No longer blocks: metrov1defect&change
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.
Flags: needinfo?(netzen)
Flags: needinfo?(hello)
QA Contact: jbecerra
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.
Flags: needinfo?(netzen)
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.
Flags: needinfo?(hello)
Ya p=1 is fine, points are not really about time at all, just estimated level of effort that one would spend fixing it.
Whiteboard: [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=0 → [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1
Blocks: 877765
Attached patch patch v2 (obsolete) — Splinter Review
Replaces use of content-navigator xbl with appbar xbl, removes various bit of older code, and fixes all of the alignment issues.
Attachment #761155 - Attachment is obsolete: true
Attached patch patch v2.1Splinter Review
Attachment #761591 - Attachment is obsolete: true
Attachment #761593 - Flags: review?(mbrubeck)
New patch was posted because the old one added spaces on blank lines.
Comment on attachment 761593 [details] [diff] [review]
patch v2.1

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

Thanks!
Attachment #761593 - Flags: review?(mbrubeck) → review+
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.
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/65cc101ff835
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Whiteboard: [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1 → [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1 status=verified
Depends on: 892224
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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.