Closed Bug 687297 Opened 9 years ago Closed 7 years ago
language-specific minimum font size is propagated to pages of different language in same window
286 bytes, text/html
285 bytes, text/html
281 bytes, text/html
7.60 KB, patch
|Details | Diff | Splinter Review|
6.75 KB, patch
|Details | Diff | Splinter Review|
Using the following settings: - Minimum font size for Western languages: None - Minimum font size for Japanese: 24 Visit a Shift-JIS encoded page (the one in the attachment for example), then, in the same tab, open any other non-Shift-JIS page: the minimum font size of 24 is carried over for the rest of the session, regardless of the encoding of the page.
Attachment #560743 - Attachment mime type: text/plain → text/html
Confirmed http://hg.mozilla.org/mozilla-central/rev/85fb038d1dd1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110917 Firefox/9.0a1 ID:20110917121100 Regression window(cached m-c hourly), Works: http://hg.mozilla.org/mozilla-central/rev/c56df99c1c75 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre ID:20110310150634 Fails: http://hg.mozilla.org/mozilla-central/rev/d4f00dcf1bb8 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre ID:20110310203732 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c56df99c1c75&tochange=d4f00dcf1bb8 Regressed by: d4f00dcf1bb8 Brad Lassey — bug 623820 - Text zoom reflow messes up layout on AMO and other sites r=roc,dbaron a=blocking-fennec
Comment on attachment 560743 [details] shift-jis ><!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> ><html> ><head> ><meta http-equiv="Content-Type" content="text/html; charset=Shift_JIS"> ><title>sdrs</title> ></head> ><body> > ></body> ></html>
Attachment #560743 - Attachment is obsolete: true
Bug 623820 was about propagating the minimum font size from one page to the next in the same tab. I guess it's not clear how that should interact with per-script minimum font size settings. Maybe we should only propagate the minimum font size when pages have the same script.
I think we should be propagating the set-from-the-outside min font size from page to page, but not the max of that with the language-specific pref. (See also comments I made in bug 902092.)
This should be fixed by revising the fix to bug 623820 (https://hg.mozilla.org/mozilla-central/rev/d4f00dcf1bb8) by: * clearly separating the min font size from the pref, the extra min font size from the container, and their combination, for example, by renaming mMinFontSize -> mExtraMinFontSize, SetMinFontSize -> SetExtraMinFontSize, etc. (Maybe "Extra" isn't the best name, but I haven't yet thought of a better one.) * adding a getter for the extra min font size in addition to the getter for their combination * making DocumentViewerImpl::GetMinFontSize and TransferZoomLevels in nsDocument.cpp call that new getter instead of the getter for the combination. (Also, please check to see if there are other new callers.) * adding an automated test for this bug (probably a mochitest)
OS: Windows 7 → All
Hardware: x86 → All
Summary: Minimum font size setting carried over after visiting a non UTF-8 encoded page → language-specific minimum font size is propagated to pages of different language in same window
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #9) > * clearly separating the min font size from the pref, the extra min font > size from the container, and their combination, for example, by renaming > mMinFontSize -> mExtraMinFontSize, SetMinFontSize -> SetExtraMinFontSize, > etc. (Maybe "Extra" isn't the best name, but I haven't yet thought of a > better one.) > > * adding a getter for the extra min font size in addition to the getter for > their combination Oops, I should have been clearer that for these two points, I'm talking specifically about the code in nsPresContext.
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
V1 Fix: Bug 687297 - language-specific minimum font size is propagated to pages of different language in same window To correct this bug, the minimum font size is propagated to pages without first being max’ed with the language-specific minimum font size preference. A separate accessor has been added to nsPresContext to enable TransferZoomLevels and nsDocumentViewer::GetMinFontSize to access the minimum font size that has not been Max’ed against the language specific minimum font size preset. nsPresContext::mMinFontSize, its getter, and its setter have been renamed to more clearly distinguish them from minFontSize used elsewhere.
Attachment #8381571 - Flags: review?(dbaron)
Comment on attachment 8381571 [details] [diff] [review] V1 proposed fix for bug 687297 >V1 Fix: Bug 687297 - language-specific minimum font size is propagated to pages of different language in same window Drop the "V1 Fix: ", and change the rest of the line to describe the fix rather than describing the bug. > /** > * Get the minimum font size for the specified language. If aLanguage >- * is nullptr, then the document's language is used. >+ * is nullptr, then the document's language is used. This is the >+ * effective minimum font size, taking into consideration the >+ * language font size preference. > */ Instead of the second sentence, I'd say something more like "This combines the language-specific global preference with the per-presentation base minimum font size." >+ /** >+ * Get the base minimum font size. This size is independent of the >+ * language font size preference. >+ */ Perhaps make this comment describe both the getter and the setter? >- >- mMinFontSize = aMinFontSize; >+ >+ mBaseMinFontSize = aMinFontSize; Don't insert trailing whitespace. >- int32_t mMinFontSize; // Min font size, defaults to 0 >- float mTextZoom; // Text zoom, defaults to 1.0 >- float mFullZoom; // Page zoom, defaults to 1.0 >+ // Base minimum font size, independent of the language font size preference. Defaults to 0 >+ int32_t mBaseMinFontSize; >+ >+ float mTextZoom; // Text zoom, defaults to 1.0 >+ float mFullZoom; // Page zoom, defaults to 1.0 Leave the comment indentation alone, and don't insert trailing whitespace. Looks good with those things addressed. You should address them, and upload a new patch (including "r=dbaron" at the end of the first line of the commit message). Also see https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in But this also shouldn't land until you also have the test.
Attachment #8381571 - Flags: review?(dbaron) → review+
Updated with dbaron's review feedback.
Attachment #8382430 - Flags: review?(dbaron)
Comment on attachment 8382430 [details] [diff] [review] V2 proposed fix for bug 687297 r=dbaron (but normally if you get review+, you don't need to ask for review again if you've done what the comments ask for. You should also mark the previous patch as obsolete, though if you use hg bzexport, bzexport will do that for you.)
Attachment #8382430 - Flags: review?(dbaron) → review+
Added a Mochitest for this bug
Comment on attachment 8382531 [details] [diff] [review] bug687297_test.patch A few things: I think you should use SpecialPowers.pushPrefEnv rather than SetIntPref, because preference changes are asynchronous on platforms that have Web content in a separate process ("electrolysis", "e10s"). This is similar, but uses a callback for when the pref change is known to be done. I think it's actually also worth testing that the middle frame has a different size. Given that you have a test for the functionality, may as well test the things that happen. (It might be easier to have each frame cal a function on the parent rather than pass information around in window.name.) Also, try to not end up with "No newline at end of file"; hg uses the Unix convention of LF as line *terminator* (rather than the Windows convention of CR+LF as line *separator*). On some platforms editors might tend to produce files without the final newline. Otherwise this looks good, though. (Does it appear in a separate window or in a separate tab?) Probably worth pushing to try server to see that it works across platforms, though.
Attachment #8382531 - Flags: review?(dbaron) → review-
bug687297_test.patch has been updated based on David Baron's review feedback.
Comment on attachment 8382626 [details] [diff] [review] Mochitest for Bug 687297 r=dbaron
Attachment #8382626 - Flags: review?(dbaron) → review+
Try push: https://tbpl.mozilla.org/?tree=Try&rev=58c06fbea0a1 (against a rather current mozilla-inbound, and thus including a bunch of revisions that hadn't been pushed to try by anyone before)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #20) > https://hg.mozilla.org/integration/mozilla-inbound/rev/27f21343ffe5 > https://hg.mozilla.org/integration/mozilla-inbound/rev/1d16baf1f90a Hi sorry, had to back this out for errors like https://tbpl.mozilla.org/php/getParsedLog.php?id=35336543&tree=Mozilla-Inbound
https://tbpl.mozilla.org/?rev=b0e36a2e2ee5&tree=Mozilla-Inbound shows the failure was only in the B2G desktop builds, which is interesting. Although I wouldn't necessarily assume the test was running in the B2G emulator builds, since there's a lot of out-of-band test disabling for b2g. Debugging that failure looks painful; I'm not sure it's necessarily worthwhile on its own, but it probably is as part of making B2G desktop tests more reliable.
The Mochitest for this bug is now excluded from the B2G Desktop Client tests, due to bug 948948.
Comment on attachment 8383782 [details] [diff] [review] Mochitest for Bug 687297 r=dbaron
Attachment #8383782 - Flags: review?(dbaron) → review+
Assignee: nobody → kgilbert
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.