Closed Bug 277278 Opened 16 years ago Closed 15 years ago

GUI for "always use my fonts" (browser.display.use_document_fonts)

Categories

(Camino Graveyard :: Preferences, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Camino1.5

People

(Reporter: mitch, Assigned: froodian)

Details

(Keywords: fixed1.8.1)

Attachments

(10 files, 6 obsolete files)

20.54 KB, application/zip
Details
66.43 KB, image/png
Details
20.53 KB, application/zip
Details
66.48 KB, image/png
Details
23.02 KB, application/zip
Details
39.04 KB, image/png
Details
66.99 KB, image/png
Details
6.92 KB, patch
Details | Diff | Splinter Review
53.94 KB, image/png
Details
20.54 KB, application/zip
alqahira
: review+
mikepinkerton
: superreview+
Details
now that "open in tabs" has been implemented (thanks pink!), the only thing that
keeps firefox on my dock is it's ability to override the font sizes on many
pages (like gmail).  would it be possible to have camino have an "always use my
fonts" option, ala firefox?
would this just be a frontend to browser.display.use_document_fonts?  is this
preference (via about:config, and through prefs on ff) accessible to camino?
Yes that would be it.

browser.display.use_document_fonts -> 0 or 1

Defines whether the browser uses the fonts specified in the web page for
display. Set to 1 to allow web pages to use fonts specified in page.

Do you still want a front end option for this?
(In reply to comment #2)
> Yes that would be it.
> 
> browser.display.use_document_fonts -> 0 or 1
> 
> Defines whether the browser uses the fonts specified in the web page for
> display. Set to 1 to allow web pages to use fonts specified in page.
> 
> Do you still want a front end option for this?

yes please, as the about:config in the nightly builds appears to be broken (at
least with the 03/06/2005 build) - it displays nothing.  i also think that it
would be good to mirror the checkbox mentality that ff has for this pref.  i
know that for me, it helps me set the fonts a little larger for reading.  is
this a trivial add?  i wouldn't mind setting the config option myself, however,
i am sure that the checkbox could benefit others...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Camino1.2
Summary: an "always use my fonts" option would be grand → GUI for "always use my fonts" (browser.display.use_document_fonts)
Now that about:config works fine, do we still want this? I say we should probably do it along with the font prefs reorganisation outlined in bug 184102 and bug 175651.

We really ought to do all three at once, though.

cl
Whiteboard: [good first bug]
I'm not sure we do, but someone could convince me otherwise.

This pref is documented on cb.o: http://www.caminobrowser.org/support/hiddenprefs/#UseMyFonts
QA Contact: preferences
There's GUI for "Always use my page and link colors", why not fonts as well?  Both are about page useability.
* about:config is bad UI :-)
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/457b9017eeddee26/2b126b0b4bac15c9#2b126b0b4bac15c9

* what Stuart says (comment #6).
Seeing the proposals in bug 184102, a small checkbox at the bottom of the font-panel would not make the UI anymore confusing or complex.

The option exist in IE Windoze, IE Mac, Firefox (and SeaMonkey), and Netscape 4 (iirc).

People who need this kind of options most are usually  not the most advanced users. Don't ask my *geeky* 77 years old neighbour to go to about:config, or worse, add a line to user.js/prefs.js.

PS -  Better use positive wording: 'Always use my fonts' is better than what Fx uses currently, I think.
To be clear, I'm in favour of a GUI option here, for the reasons Stuart and Philippe mention, and I'm especially in favour of doing it if we're going to be doing a major Appearance prefs overhaul.

cl
Assignee: mikepinkerton → stridey
Attached patch Patch (obsolete) — Splinter Review
Not sure if doing this was supposed to wait on other appearance prefpane bugs, but...
Attachment #225221 - Flags: review?(hwaara)
Attached file Proposed nib change
I wonder if the checkbox should go under Advanced...
(In reply to comment #12)
> I wonder if the checkbox should go under Advanced...
> 

Just to play devil's advocate, it isn't under Advanced in the other half of the Appearance pref pane. (Granted, there's no "Advanced" option at all for the Colors tab, but still...seems like that would make it undiscoverable.)

Ian, as far as I'm concerned, go ahead and fix this, and we'll fix the nib while we're touching it. Someone should file another bug for all our other nib changes if it hasn't been filed already. (We also need to agree on some sort of consistent spacing.)

cl
Comment on attachment 225221 [details] [diff] [review]
Patch

Can you just put some kind of comment somewhere indicating that a value of zero for that pref means we use only the user fonts?

That had me confused for a few seconds.  Looks good otherwise.
Attachment #225221 - Flags: review?(hwaara) → review-
Attached patch Better documented Patch (obsolete) — Splinter Review
Requesting addition r from cl
Attachment #225221 - Attachment is obsolete: true
Attachment #225270 - Flags: review?(bugzilla)
Attachment #225270 - Flags: review?(hwaara)
Comment on attachment 225270 [details] [diff] [review]
Better documented Patch

Assuming the UI change looks good. Code is ok.
Attachment #225270 - Flags: review?(hwaara) → review+
I think it should be in Advanced (since it's a special-use, and potentially site-breaking, pref), but the current prefs in the Advanced sheet are all by-region, and this one is global.  Yet another reason that prefPane sucks.

Since reset is (currently) global, this pref makes more sense positioned between "Advanced" and "Reset" for the time being.

Also, IMO, the string should be more descriptive, something like "Always use my fonts instead of the ones specified by the page."

(I'm *still* trying to think of ways to fix that prefPane's UI....)
Attached file Option 2 nib change
The real problem here (nibwise) is that the whole prefpane sucks (bug 184102).  I think the best solution for the time being is to put it below the advanced button like Smokey says as a temporary solution.  The problem with putting it in Advanced IMO is that a)Advanced is all region-specific (except for this), and b)It would hurt discoverability.

I think the name should remain the same, for parity with the "Use my colors" pref.  If it should be more descriptive, they can both be changed at the same time when 184102 gets fixed.
Comment on attachment 225270 [details] [diff] [review]
Better documented Patch

Looks fine to me with the comments r+
Attachment #225270 - Flags: review?(bugzilla) → review+
While we're looking at this, should "advanced" in the hint text be "Advanced" instead, to match the button label?

Also, technically the "Always use my fonts" should be spaced above the "Reset" rather than sharing space with it; however, doing so will cause the prefPane to expand vertically and therefore require re-layout of the Colors tab, too :(  Don't know what we want to do here....
Hrm.  Maybe putting it in Advanced is the cleanest (least dirty?) solution.  Here's what that looks like.

And yeah, that hint text should be "Advanced."  If we decide on one of the main prefpane solutions, I'll whip up a new nib incorporating that.
Attachment #225270 - Flags: superreview?(mikepinkerton)
+// The exposed pref has reverse logic from the internal pref,
+// so mUseMyFontsCheckbox = 1 maps to use_document_fonts = 0 

can you also make a comment about this when you set the pref when the panel loads? people might not see the later comment and stop and debug it. best to be clear. Or, better yet, provide a small wrapper function and all the setters can use that. Maybe that's overkill. Thoughts?
Attached patch Patch (obsolete) — Splinter Review
This hides all the 0 == 1 logic away in useMyFontsPref: and setUseMyFontsPref:.  As you say Mike, it may be overkill (in which case I can just add more comments to what I had before), but I like it since it allows all the code elsewhere to pretend the pref has the same logic as what gets exposed in the UI.
Attachment #225270 - Attachment is obsolete: true
Attachment #225750 - Flags: superreview?(mikepinkerton)
Attachment #225270 - Flags: superreview?(mikepinkerton)
Attached file #camino developed nib (obsolete) —
This nib keeps the checkbox out of the Advanced sheet, and attempts to follow the guidelines in http://wiki.caminobrowser.org/Development:Reviewing
Attachment #225820 - Flags: review?(alqahira)
+  [self clearPref:"browser.display.use_document_fonts"];

what's the purpose of this?
Comment on attachment 225750 [details] [diff] [review]
Patch

duh sr=pink
Attachment #225750 - Flags: superreview?(mikepinkerton) → superreview+
(In reply to comment #27)
> Created an attachment (id=225821) [edit]
> Screenshot of the colors/links side of latest nib

I don't like the spacing. The "Underline links" checkbox should be closer to the links color swatches than it is to the "Always..." checkbox, because it's link-related. Don't touch the current layout.
Oops.  I made the changes in Appearance.h but forgot to diff it.  Thanks to smokey for trying to build with the patch and catching this. *embarrassed*
Attachment #225750 - Attachment is obsolete: true
Just like the last one, but without f***ing up the whitespace.
Attachment #225961 - Attachment is obsolete: true
(In reply to comment #31)
> I don't like the spacing. The "Underline links" checkbox should be closer to
> the links color swatches than it is to the "Always..." checkbox, because it's
> link-related. Don't touch the current layout.

I agree that the current layout should be disturbed as little as possible, but if we're going to put the new pref in the main Fonts tab, we're gonna have to do a little bit of reflow on the colors & links tab to keep it OK looking.  How would you feel about a nib that had the same Fonts side as the last posted, and has a Colors & Fonts tab like this?

This keeps the same number of px between The "Underline links" checkbox and the links color swatches.  It just expands the distance between the two sets of color swatches, the two checkboxes, and adds some whitespace above the reset button.
Attachment #225820 - Attachment is obsolete: true
Attachment #225966 - Flags: review?(alqahira)
Attachment #225820 - Flags: review?(alqahira)
Attachment #225821 - Attachment is obsolete: true
Comment on attachment 225966 [details]
#camino developed nib, version 2

This looks nice and works OK for me (less bug 278820, of course).

Simon, is the Links & Colors side in attachment 225964 [details] OK with you?
Attachment #225966 - Flags: superreview?(mikepinkerton)
Attachment #225966 - Flags: review?(alqahira)
Attachment #225966 - Flags: review+
Comment on attachment 225966 [details]
#camino developed nib, version 2

sr=pink
Attachment #225966 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [good first bug] → [needs checkin]
fixed on trunk and 1.8 branch
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
(In reply to comment #28)
> Created an attachment (id=225822) [edit]
> Screenshot of the fonts side of latest nib
> 
Hmm, I'm not really crazy about the "advanced" tip text that we have, in part because it uses the word "you", which I think is a little weird for a UI. No?
Here's my suggestion: « To set the minimum font size and other font types for this region, click Advanced... »
Also, this would go above the button, rather than below it.
What do you good people think?
This bug has been fixed.  File a new bug for any issues you have.
Verified on 1.8 branch and trunk
Status: RESOLVED → VERIFIED
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.
Target Milestone: Camino1.2 → Camino1.1
You need to log in before you can comment on or make changes to this bug.