Closed Bug 1008169 Opened 10 years ago Closed 9 years ago

Font selection and font size dropdowns are reacting very slowly on press up/down

Categories

(Firefox :: Settings UI, defect, P3)

Product:
Firefox
Component:
Settings UI
Version:
31 Branch
Type:
defect
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 41
Iteration:
41.1 - May 25
Tracking Flags:
Tracking Status
firefox41 --- verified

People

(Reporter: whimboo, Assigned: Gijs)

References

Details

(Keywords: perf, Whiteboard: [sf-hackweek])

Attachments

(2 files, 3 obsolete files)

Patch
3.05 KB, patch
Gijs
: review-
Details | Diff | Splinter Review
MozReview Request: Bug 1008169 - Font selection and font size dropdowns are reacting very slowly on press up/down,r?jaws
39 bytes, text/x-review-board-request
jaws
: review+
Details 
In the Content pane of about:preferences, when you select font selection or size dropdown, the update is happening very slowly. 

1. Open about:preferences
2. Go to the content pane
3. Hit accesskey D to select the font selection dropdown
4. Press up and down keys multiple times

With step 4 you will notice a 1 second delay until the next font gets selected.
Peregrino, can you take this? I think we can fix this by disabling instantApply while the font-selection dropdown is open.
Flags: needinfo?(colmeiro)
Yeah, I'll take a look
Assignee: nobody → colmeiro
Flags: needinfo?(colmeiro)
Removing from blocking since the severity is low and the frequency of occurrence is low.
No longer blocks: ship-incontent-prefs
Peregrino, are you planning on picking this back up or should I unassign it?
Flags: needinfo?(colmeiro)
Whiteboard: [sf-hackweek]
Assuming Peregrino is busy, and unassigning for now. Let us know if we're wrong!
Assignee: colmeiro → nobody
Flags: needinfo?(colmeiro)
Attached patch Patch (obsolete) — — Splinter Review 
Assignee: nobody → jaws
Status: NEW → ASSIGNED

        Attachment #8579513 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8579513 [details] [diff] [review]
Patch

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

Per rl discussion, it would be better if we could just make this do the right thing rather than avoiding doing everything it's currently doing.

        Attachment #8579513 -
        Flags: review?(gijskruitbosch+bugs) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1.1 (obsolete)
        — 
          — Splinter Review
      

    


  
Through some more adhoc testing it appears that the cost here comes from applying these changes to all of the open tabs in the current session. I've made the previous patch simpler but stayed with the initial approach.

        Attachment #8579513 -
        Attachment is obsolete: true

        Attachment #8579624 -
        Flags: review?(gijskruitbosch+bugs)

    
  

        Attachment #8579624 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
https://hg.mozilla.org/integration/fx-team/rev/ba4279ef4045
Keywords: checkin-needed
Whiteboard: [sf-hackweek] → [sf-hackweek][fixed-in-fx-team]

    
    

    
  
I just looked at this again, and now I'm confused. How does the blur ever fire on the preference element for the language group? :-\
Flags: needinfo?(jaws)

    
    

    
  
I'm going to double-check/verify this when it hits Nightly.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ba4279ef4045
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-firefox39:
          --- → fixed
Resolution: --- → FIXED
Whiteboard: [sf-hackweek][fixed-in-fx-team] → [sf-hackweek]
Target Milestone: --- → Firefox 39

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #11)
> I just looked at this again, and now I'm confused. How does the blur ever
> fire on the preference element for the language group? :-\

This didn't fix it, we'll need to get a better fix here.

Backed out, https://hg.mozilla.org/integration/fx-team/rev/db20ba3a49d2
Flags: needinfo?(jaws)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 39 → ---
Points: --- → 5
Priority: -- → P3

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch
        — 
          — Splinter Review
      

    


  

        Attachment #8579624 -
        Attachment is obsolete: true

        Attachment #8593494 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8593494 [details] [diff] [review]
Patch

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

r- because of the multiple-elements-in-a-pane issue.

I wonder if we should make this opt-in somehow...

::: toolkit/content/widgets/preferences.xml
@@ +1265,5 @@
>        
> +      <field name="DeferredTask" readonly="true">
> +        let targetObj = {};
> +        Components.utils.import("resource://gre/modules/DeferredTask.jsm", targetObj);
> +        targetObj.DeferredTask;

I wonder if it's worth making this a lazy getter.

@@ +1272,5 @@
>        <method name="userChangedValue">
>          <parameter name="aElement"/>
>          <body>
>          <![CDATA[
> +          if (!this._persistPreferenceDeferredTask) {

This will create the task regardless of whether this is an element with a preference associated with it.

The other thing that I believe is wrong here is that this method lives on the prefpane. It could thus be called twice for different elements (with a different aElement), and because of the caching here it will always save the value for the first element it gets called about, not any of the others.

I also anticipate making this async will cause test failures, but I'm not 100% sure about that.

        Attachment #8593494 -
        Flags: review?(gijskruitbosch+bugs) → review-
Assignee: jaws → nobody
Status: REOPENED → NEW

    
  
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  

      
      
      
      

        Attached file
          
        MozReview Request: bz://1008169/Gijs (obsolete)
        — 
      

    


  
/r/8411 - Bug 1008169 - Font selection and font size dropdowns are reacting very slowly on press up/down,r?jaws

Pull down this commit:

hg pull -r 1496344a0f4c2f193e8ba311f05b76659a1671ad https://reviewboard-hg.mozilla.org/gecko/

        Attachment #8603303 -
        Flags: review?(jaws)

    
    

    
  
Same kind of solution, but keeping the task per-element, and a set on the pane so we can force-save all of them when we call writePreferences.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8603303 [details]
MozReview Request: bz://1008169/Gijs

https://reviewboard.mozilla.org/r/8409/#review7107

::: toolkit/content/widgets/preferences.xml:1304
(Diff revision 1)
> -            var prefVal = preference.getElementValue(element);
> +              element._deferredValueUpdateTask = new this.DeferredTask(this._deferredValueUpdate.bind(this, element), 1000);

I thought you were going to make this opt-in per element, instead of making all elements get this.

        Attachment #8603303 -
        Flags: review?(jaws)

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Comment on attachment 8603303 [details]
> MozReview Request: bz://1008169/Gijs
> 
> https://reviewboard.mozilla.org/r/8409/#review7107
> 
> ::: toolkit/content/widgets/preferences.xml:1304
> (Diff revision 1)
> > -            var prefVal = preference.getElementValue(element);
> > +              element._deferredValueUpdateTask = new this.DeferredTask(this._deferredValueUpdate.bind(this, element), 1000);
> 
> I thought you were going to make this opt-in per element, instead of making
> all elements get this.

Good point!
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8603303 [details]
MozReview Request: bz://1008169/Gijs

/r/8411 - Bug 1008169 - Font selection and font size dropdowns are reacting very slowly on press up/down,r?jaws

Pull down this commit:

hg pull -r 7ae9afb43ab0babd035f0b729bb4e9bac04f1a26 https://reviewboard-hg.mozilla.org/gecko/

        Attachment #8603303 -
        Flags: review?(jaws)

    
  
Flags: needinfo?(gijskruitbosch+bugs)

        Attachment #8603303 -
        Flags: review?(jaws) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e734bdd5e82b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago

          status-firefox41:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Iteration: 40.3 - 11 May → 41.1 - May 25
Flags: qe-verify?
Flags: firefox-backlog+

    
  
Flags: qe-verify?
Flags: qe-verify+
Flags: in-testsuite-

    
  
Depends on: 1169567
QA Contact: camelia.badau

    
    

    
  
Verified fixed on Windows 7 64bit and Ubuntu 13.10 32bit using latest Nightly 41.0a1 (buildID: 20150602055237).
Status: RESOLVED → VERIFIED

          status-firefox41:
          fixed → verified

    
    

    
  

        Attachment #8603303 -
        Attachment is obsolete: true

        Attachment #8618158 -
        Flags: review+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: