Closed Bug 919340 Opened 11 years ago Closed 11 years ago

Searching through the quickfind bar should focus links

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27
Tracking Flags:
Tracking Status
firefox25 --- unaffected
firefox26 - unaffected
firefox27 + fixed

People

(Reporter: simon.lindholm10, Assigned: evilpie)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

highligh-test
2.44 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
highlight-link v3
8.02 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
highlight-remote
2.65 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20130922030201

Steps to reproduce:

Open xkcd.com, open the quickfind bar through '/', and type "blag".


Actual results:

The "blag" link on the page gets highlighted, like any other search result, but does not receive a focus outline (until escape is pressed).


Expected results:

Should get focus automatically, to make it possible to use the link automatically by pressing enter (or, in the "links only" case, to make that possibility clearer).

Relatedly, pressing tab should move focus to the next element after the link, and hide the findbar. Right now it focuses the element itself, and does not hide the findbar.
Recent regression, so probably bug 666816.
Blocks: 666816
Keywords: regression
status-firefox25: --- → unaffected
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox26: --- → ?
tracking-firefox27: --- → ?
Version: 27 Branch → Trunk
Attached patch highlight-link (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 808637 [details] [diff] [review]
highlight-link

It seems like before we also highlighted found links, even when we weren't in link only mode. I wish we could somehow remove all the special keypress and focus handling, that we have to do, because the findbar behaves in weird ways. I will see if I can at least write a test for highlighting links and clicking them.

        Attachment #808637 -
        Flags: review?(mdeboer)

    
  
Status: UNCONFIRMED → NEW
Ever confirmed: true

        Attachment #808637 -
        Attachment is patch: true

    
    

    
  
Comment on attachment 808637 [details] [diff] [review]
highlight-link

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

This patch causes the highlight-links feature to also be on while NOT using quick find, which I think is a regression?

Also you mentioned writing a (regression-)test for this. I can only say that that would be highly appreciated!!!

        Attachment #808637 -
        Flags: review?(mdeboer) → feedback+

    
    

    
  
Oh damn, no my memory came back. I was using aLinksOnly as a cheap indicator for quickfind, because I didn't think I would have to actually pass whether it is a quickfind operation. This has been like this since one of the first pages and I totally forgot about it.

    
    

    
  
s/no my/now my/ s/pages/patches

Don't write comments a few minutes after you wake up.

    
  
Assignee: nobody → evilpies
Status: NEW → ASSIGNED

    
    

    
  
I wrote a new patch and a test :) Currently running it through try: https://tbpl.mozilla.org/?tree=Try&rev=aa4d156478ee

    
    

    
  

      
      
      
      

        Attached patch
          
          
        highlight-link v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #808637 -
        Attachment is obsolete: true

        Attachment #809900 -
        Flags: review?(mdeboer)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        highligh-test
        — 
          — Splinter Review
      

    


  
I have to admit that unsing browser.finder to test this is probably not the most faithful way. However it's much easier and let's me test different search parameters quite easy.

        Attachment #809902 -
        Flags: review?(mdeboer)

    
    

    
  
Comment on attachment 809900 [details] [diff] [review]
highlight-link v2

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

::: toolkit/modules/Finder.jsm
@@ +173,3 @@
>        return;
>  
> +    if (this._previousLink) {

This now duplicates code at L111. Please unify this to a function called `_restoreOriginalOutline`

        Attachment #809900 -
        Flags: review?(mdeboer) → review-

    
    

    
  
Apart from that small remark, the patch looks good to me!

    
    

    
  


  
Thank you for the immediate review!

        Attachment #809900 -
        Attachment is obsolete: true

        Attachment #809931 -
        Flags: review?(mdeboer)

    
    

    
  
Comment on attachment 809902 [details] [diff] [review]
highligh-test

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

LGTM, apart form a couple of small nits. I actually like the unit separation for testing these features.

::: toolkit/modules/tests/browser/browser_Finder.js
@@ +8,5 @@
>  
>  function test () {
>    waitForExplicitFinish();
>  
> +  tab = gBrowser.addTab("data:text/html,<body><iframe srcdoc='content'></iframe><br/><a href='http://test.com'>test link</a>");

nit: please break this into proper-width chunks and can you use escaped double quotes?

@@ +18,5 @@
>  
> +var outlineTest = "sendAsyncMessage('OutlineTest', " +
> +                  "{ ok : !!content.document.getElementsByTagName('a')[0].style.outline }" +
> +                  ");";
> +

nit: please outline this properly and can you use escaped double quotes here too?

@@ +54,5 @@
> +        })
> +        browser.messageManager.loadFrameScript("data:," + outlineTest, false)
> +      }
> +      finder.fastFind("test link", true, true); // search for link and draw outline
> +      finder.fastFind("test link", false, false); // just search for "test link"

nit: please put the comments above the statements, neatly capitalized.

        Attachment #809902 -
        Flags: review?(mdeboer) → review+

        Attachment #809931 -
        Flags: review?(mdeboer) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        highlight-remote
        — 
          — Splinter Review
      

    


  
Simple patch fixes e10s.

        Attachment #810688 -
        Flags: review?(mdeboer)

    
    

    
  
Comment on attachment 810688 [details] [diff] [review]
highlight-remote

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

Hmm, yes, this is rather straightforward stuff...

        Attachment #810688 -
        Flags: review?(mdeboer) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ba4baeace8dd
https://hg.mozilla.org/mozilla-central/rev/31fc330fb7fc
Status: ASSIGNED → RESOLVED
Closed: 11 years ago

          status-firefox27:
          affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

    
    

    
  
uplift nomination for FF26 please, with risk assessment.
Flags: needinfo?(evilpies)

    
    

    
  
Comment on attachment 809931 [details] [diff] [review]
highlight-link v3

I didn't realize that we made it to Beta already :/

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816 
User impact if declined: breaks user facing feature
Testing completed (on m-c, etc.): m-c / Aurora
Risk to taking this patch (and alternatives if risky): a tad more complex than the rest, but nothing to worry about
String or IDL/UUID changes made by this patch:

        Attachment #809931 -
        Flags: approval-mozilla-beta?

    
    

    
  
Wait, maybe we are on Aurora after all? I am confused sorry
Flags: needinfo?(evilpies) → needinfo?(lsblakk)

    
    

    
  
Comment on attachment 809931 [details] [diff] [review]
highlight-link v3

Yes, 27 is already on Aurora, let's get this into the early Beta.

        Attachment #809931 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(lsblakk)

    
    

    
  
This has some pretty major conflicts with beta. Please attach a branch-specific patch.
Flags: needinfo?(evilpies)
Keywords: branch-patch-needed
Flags: needinfo?(evilpies)

    
    

    
  
Is an uplift even relevant given bug 916536?

    
    

    
  
Lukas, the bugs that block bug 666816 contain patches to - among other files - `toolkit/modules/Finder.jsm`, which is not yet in the mozilla-beta tree.

Since we backed this out from beta, I think that after an uplift to Aurora of this patch (and others), we're good to go and riding the train comfortably. Am I right?
Flags: needinfo?(lsblakk)

        Attachment #809931 -
        Flags: approval-mozilla-beta+

    
    

    
  
Yes, thanks for putting that all together succinctly, since 666816 was backed out of 26 in bug 916536 I'm marking this bug unaffected on 26 and removing tracking, we should be fine with this in 27.

    
    

    
  
Well that is nice. :)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: