https://bugzilla.mozilla.org/show_bug.cgi?id=1687635 Bugzilla Quick Search [ ] * Browse * Advanced Search * Reports * * Documentation * New Account * Log In Login with GitHub [ ] [ ] [password ] [*] Remember [Log in] * Forgot Password [ ] [Reset Password] [moz] * Mozilla Home * * Privacy * Cookies * Legal Please enable JavaScript in your browser to use all the features on this site. Copy Summary [ ] View V * Reset Sections * Expand All Sections * Collapse All Sections * * History * * JSON * XML Closed Bug 1687635 Opened 1 year ago Closed 9 months ago Replace the Text Encoding menu with a single override item Repair Text Encoding Summary: Replace the Text Encoding menu with a single override item Repair Text Encoding Categories (Core :: Internationalization, enhancement) Product: Core V Core Shared components used by Firefox and other Mozilla software, including handling of Web content; Gecko, HTML, CSS, layout, DOM, scripts, images, networking, etc. Issues with web page layout probably go here, while Firefox user interface issues belong in the Firefox product. (More info) See Open Bugs in This Product File New Bug in This Product Watch This Product Component: Internationalization V Core :: Internationalization Internationalization is the process of designing and developing a software product to function in multiple locales. This process involves identifying the locales that must be supported, designing features which support those locales, and writing the code needed. See Open Bugs in This Component Recently Fixed Bugs in This Component File New Bug in This Component Watch This Component Version: unspecified Platform: Unspecified Unspecified Type: enhancement Priority: Not set Severity: -- Points: --- Tracking () Status: RESOLVED FIXED Status: RESOLVED FIXED Mark as Assigned Milestone: 91 Branch Iteration: --- Project Flags: Root Cause [---] a11y-review [---] Performance [---] Webcompat Priority [---] user-doc-firefox [---] Fission Milestone [---] Tracking Flags: Tracking Status firefox91 --- fixed Tracking Status relnote-firefox [---] thunderbird_esr91 [---] [---] firefox-esr91 [---] [---] firefox91 [fixed] firefox97 [---] [---] firefox98 [---] [---] firefox99 [---] [---] People (Reporter: hsivonen, Assigned: hsivonen) Assignee: # hsivonen Assignee: [ ] Reset Assignee to default Mentors: --- QA Contact: [ ] Reset QA Contact to default Reporter: # hsivonen Triage Owner: # m_kato CC: 13 people References Depends on: 1696582 1648464 1686463, 1704056 Blocks: 1701828 1704749, 1713627 Dependency tree / graph Regressions: --- Regressed by: --- URL: See Also: 1725937 Details Alias: --- Keywords: --- Whiteboard: --- QA Whiteboard: --- Has Regression Range: --- Has STR: --- Votes: 1 Bug Flags: behind-pref [ ] firefox-backlog [] sec-bounty [ ] sec-bounty-hof [] in-qa-testsuite [ ] [ ] in-testsuite [ ] qe-verify [] Crash Data Signature: Security (public) This bug is publicly visible. User Story Attachments (3 files, 1 obsolete file) Bug 1687635 part 1 - Add new localizable strings for Override Text Encoding. Details | 10 months ago Review Henri Sivonen (:hsivonen) 48 bytes, text/x-phabricator-request Bug 1687635 part 1 - Replace Text Encoding submenu with Repair Text Encoding item. Details | 10 months ago Review Henri Sivonen (:hsivonen) 48 bytes, text/x-phabricator-request Bug 1687635 part 2 - Disable Repair Text Encoding when known not to have effect. Details | 10 months ago Review Henri Sivonen (:hsivonen) 48 bytes, text/x-phabricator-request #Screenshots: Old on the left, new on the right 9 months ago Details Henri Sivonen (:hsivonen) 437.10 KB, image/png Show Obsolete Bottom | Tags V * Reset Timeline V * Reset * * Collapse All * Expand All * Comments Only Henri Sivonen (:hsivonen) Assignee [003] Description * 1 year ago Subject to telemetry results from bug 1648464, replace the Text Encoding menu with a single menu item called Override Text Encoding that performs the action currently performed by the item Automatic in the Text Encoding menu. Benefits: * User doesn't need to know which item to choose. * Removal of one level of submenus, which might allow the new single item to remain in the hamburger menu. * Harder to get the user to self-XSS, because the attack needs to fool the detector instead of stating a particular encoding to the user. * Allows for even more conditions where the item can be disabled (since there will be more cases where we know the item won't change anything), which saves users' time by not allowing the feature to be activated when useless. Masatoshi Kimura [:emk] Updated [e93] * 11 months ago Depends on: 1696582 Henri Sivonen (:hsivonen) Assignee [003] Comment 1 * 11 months ago A look at the relevant telemetry: https://hsivonen.fi/ encoding-telemetry/ Henri Sivonen (:hsivonen) Assignee [003] Updated * 11 months ago Depends on: 1704056 Henri Sivonen (:hsivonen) Assignee [003] Updated * 11 months ago Blocks: 1701828 Henri Sivonen (:hsivonen) Assignee [003] Comment 2 * 10 months ago Additional non-user-facing benefit: * The back end code for supporting the non-Automatic menu items got in the way of implementing bug 673087. Based on that experience, chances are that fixing bug 1701828 would be much easier if we first implemented the change proposed here. Henri Sivonen (:hsivonen) Assignee [003] Comment 3 * 10 months ago Attached file Bug 1687635 part 1 - Add new localizable strings for Override Text Encoding. (obsolete) -- Details Phabricator Automation Updated [a08] * 10 months ago Attachment #9217362 - Attachment description: Bug 1687635 part 1 - Add new localizable strings. - Bug 1687635 part 1 - Add new localizable strings for Override Text Encoding. Henri Sivonen (:hsivonen) Assignee [003] Comment 4 * 10 months ago Attached file Bug 1687635 part 1 - Replace Text Encoding submenu with Repair Text Encoding item. -- Details The changeset deliberately does not clean up the resulting dead code to make reverting easier if needed. Henri Sivonen (:hsivonen) Assignee [003] Comment 5 * 10 months ago Attached file Bug 1687635 part 2 - Disable Repair Text Encoding when known not to have effect. -- Details Henri Sivonen (:hsivonen) Assignee [003] Comment 6 * 10 months ago https://treeherder.mozilla.org/#/jobs?repo=try&revision= 97dfadd18550439cef83e0c7155a5a5fc6870b31 Henri Sivonen (:hsivonen) Assignee [003] Updated * 10 months ago Assignee: nobody - hsivonen Status: NEW - ASSIGNED Henri Sivonen (:hsivonen) Assignee [003] Comment 7 * 10 months ago https://treeherder.mozilla.org/#/jobs?repo=try&revision= 2e333f4b6299d4e97be4e924bd1099bd035e25cd Henri Sivonen (:hsivonen) Assignee [003] Updated * 10 months ago Attachment #9217363 - Flags: ui-review?(mwalkington) Henri Sivonen (:hsivonen) Assignee [003] Comment 8 * 10 months ago I intend to remove the resulting dead code as a follow-up. Phabricator Automation Updated [a08] * 10 months ago Attachment #9217363 - Attachment description: Bug 1687635 part 2 - Change UI code for Override Text Encoding. - Bug 1687635 part 1 - Replace Text Encoding submenu with Override Text Encoding item. Phabricator Automation Updated [a08] * 10 months ago Attachment #9217373 - Attachment description: Bug 1687635 part 3 - Disable Override Text Encoding when known not to have effect. - Bug 1687635 part 2 - Disable Override Text Encoding when known not to have effect. Phabricator Automation Updated [a08] * 10 months ago Attachment #9217362 - Attachment is obsolete: true Henri Sivonen (:hsivonen) Assignee [003] Comment 9 * 10 months ago https://treeherder.mozilla.org/#/jobs?repo=try&revision= ffb54adc93a676462bd4e618ee94a10ab9f7f6d4 Henri Sivonen (:hsivonen) Assignee [003] Comment 10 * 10 months ago It was pointed out to me that needinfo might be better than ui-review? these days. Flags: needinfo?(mwalkington) Henri Sivonen (:hsivonen) Assignee [003] Comment 11 * 10 months ago The ui-review question being: Is the menu item label "Override Text Encoding" OK enough to land this? Magnus Melin [:mkmelin] Updated [18e] * 10 months ago Blocks: 1704749 Meridel [:meridel] Comment 12 [3d7] * 9 months ago Hi Henri, can you help me understand where this label appears (perhaps via screenshot and trigger instructions)? Changes to menus added via the Customize pane were not in scope for MR1 and Flod advised not to make any changes to this menu. Flags: needinfo?(mwalkington) - needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee [003] Comment 13 * 9 months ago Attached image Screenshots: Old on the left, new on the right -- Details Hi Henri, can you help me understand where this label appears (perhaps via screenshot and trigger instructions)? The string "Override Text Encoding" appears in two places: * In the menubar View menu as a menu item in the place currently occupied by the Text Encoding submenu. (I.e. the submenu is replaced with a single item called Override Text Encoding whose function is the same as the current item Automatic in the current submenu.) * As the label of the toolbar button, which is currently labeled Text Encoding in toolbar customization or in the overflow menu. (Instead of the button opening a menu, with this patch clicking the button performs the operation that is currently performed by the item Automatic in the menu currently opened by the button.) The string "Guess text encoding from page content" appears as the tooltip for the toolbar button. The accelerator key, c, for the Override Text Encoding menu item in the View menu intentionally remains the same as the accelerator key for the current submenu. I generated try builds for experimentation: https://treeherder.mozilla.org/#/jobs?repo=try&revision= cd2f329ffee9e3cf540c45bc0fa419e7125ef133 Note that the menu item and button are disabled on pages where they'd do nothing. Here's an example of a page where they are enabled: https://hsivonen.com/test/charset/unlabeled-utf8/ja.htm I'm attaching an image of screenshots with the current situation on the left and the situation with the patch on the right. In the View menu, the item Automatic of the Text Encoding submenu becomes an item Override Text Encoding in the View menu directly and the submenu and the other submenu items go away. The item Automatic in the menu opened by the toolbar button becomes the action of the toolbar button itself so that that the toolbar button no longer opens a menu. When the action is not available, the toolbar button itself is disabled instead of the current state where the toolbar button is always enabled but its menu items are disabled. Flags: needinfo?(hsivonen) - needinfo?(mwalkington) Henri Sivonen (:hsivonen) Assignee [003] Comment 14 * 9 months ago (In reply to Henri Sivonen (:hsivonen) from comment #13) https://hsivonen.com/test/charset/unlabeled-utf8/ja.htm While that page enabled the feature, these are better examples in terms of what scenarios they represent: https://hsivonen.com/test/charset/mislabeled-as-utf8/ ja-Shift_JIS.html https://hsivonen.com/test/charset/mislabeled-as-legacy/ja-EUC-JP.htm Meridel [:meridel] Comment 15 [3d7] * 9 months ago Thanks, Henri. I also spoke with Flod to get a better understanding of text encoding in general. What is the feature overriding text encoding with? i.e., the page is not displaying correctly. I select "Override Text Encoding" to fix the issue, which overrides the current text (or characters?) with _____. Flags: needinfo?(mwalkington) - needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee Comment 16 [003] * 9 months ago * Edited What the user wants to do is to override the text encoding that they visually believe to be wrong with the correct one. What the menu item does is it overrides the text encoding with a guess made from the page content. Whether the verb should be what the user is trying to do (Correct, Fix) or less promising of success (Override, Guess) depends on whether we want to claim success for mechanism that is successful with a high probability (extremely high for the main uses cases) but that doesn't guarantee success. I'm shy to use a verb that over-promises, because: * There are going to be failures but very rarely for the main use case of applying the feature to non-Latin-script content. (I still believe that this change will be a net usability improvement; users aren't really that good at choosing right on the first try from the long manual list.) * Overpromising invites the question "If you know how to correct it, why don't you just do it by default?" (But see previous point.) * Experience indicates some users who use this feature believe that they know encoding stuff better than Firefox does and are upset if Firefox takes away their options when Firefox knows that none of the options that were available in an older version would work. However, in this case (see first point), there are going to be (rare) cases where this patch takes away options and the user actually could know better than Firefox. I don't want to flame users in that case. Flags: needinfo?(hsivonen) - needinfo?(mwalkington) Meridel [:meridel] Comment 17 [3d7] * 9 months ago It sounds like "Override text encoding" is technically accurate and safe. It would nice if the label could communicate the benefit of the feature without over-promising. Did you consider something like: "Troubleshoot text encoding"? Flags: needinfo?(mwalkington) - needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee [003] Comment 18 * 9 months ago (In reply to Meridel [:meridel] from comment #17) It sounds like "Override text encoding" is technically accurate and safe. It would nice if the label could communicate the benefit of the feature without over-promising. Did you consider something like: "Troubleshoot text encoding"? I hadn't considered "Troubleshoot". Now that I consider it, my first thought is that the UI items that I've seen before that have said "Troubleshoot" are more ceremonious than this one: They either launch a separate more or run something automated that takes for long enough to have a progress bar. In this case, the menu item / toolbar button just does its thing instantaneously without any further UI. If "Troubleshoot" sets the expectation of some further UI appearing and then no further UI appears, might that be confusing? Flags: needinfo?(hsivonen) - needinfo?(mwalkington) Meridel [:meridel] Comment 19 [3d7] * 9 months ago I am less concerned about "Troubleshooting" setting a false expectations of a separate or elongated process, and more that users would expect something called 'troubleshoot' to diagnose a problem. Would it be accurate to say this feature diagnoses a problem? By correcting the text encoding do they then understand the source of the issue? Documenting label options: 1. Override text encoding: Technically accurate but does not communicate the benefit of the feature 2. Troubleshoot text encoding: Gets at the benefit of the feature but could set expectations of diagnosis? 3. Fix text encoding: Communicates benefit but could over-promise. Our goal in product copy isn't to be 100% technically accurate, though, so this label may be the best option for most contexts. 4. Fix garbled text: Communicates benefit but could over-promise. Plainer language than "text encoding" but is it accurate enough? 5. Fix default text encoding: More specific than option 3 and 4, but longer FYI, if a request like this is time-sensitive and I become a blocker for shipping, please don't hesitate to reach out to me on Slack (@Meridel). Betsy and I make up the content design team and are spread thin across projects so these kind of string requests can get lost in the daily shuffle! Flags: needinfo?(mwalkington) - needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee [003] Comment 20 * 9 months ago (In reply to Meridel [:meridel] from comment #19) I am less concerned about "Troubleshooting" setting a false expectations of a separate or elongated process, and more that users would expect something called 'troubleshoot' to diagnose a problem. To get impressions from more people on various options, I asked about this in the DOM Core team meeting. The result was that "Troubleshoot" suggested a bigger thing than other options. Would it be accurate to say this feature diagnoses a problem? I guess it's accurate enough that the feature makes a partial diagnosis as part of its internal operation, but what's exposed to the user is the page became readable (likely) or the page remained unreadable (unlikely). By correcting the text encoding do they then understand the source of the issue? I expect users who use this feature to be aware of the conceptual source of the problem before invoking the UI even if they couldn't identify the exact pair of wrong and right encoding in a particular case. Documenting label options: 1. Override text encoding: Technically accurate but does not communicate the benefit of the feature 2. Troubleshoot text encoding: Gets at the benefit of the feature but could set expectations of diagnosis? "Troubleshoot" also could set expectations of a bigger operation than what's going to happen. 3. Fix text encoding: Communicates benefit but could over-promise. Our goal in product copy isn't to be 100% technically accurate, though, so this label may be the best option for most contexts. Apart from potentially over-promising, this raises the question "If you know how to fix it, why didn't you just do it already?" (This concern was raised in the DOM Core meeting.) 4. Fix garbled text: Communicates benefit but could over-promise. Plainer language than "text encoding" but is it accurate enough? I'd like to keep "Text Encoding" in the string in order to keep the term that people who've previously used the submenu have already seen and to enable the same accelerator key letter to be found in the string. The technically accurate term would be "Character Encoding", and we used to use that. However, we previously changed it from "Character Encoding" to "Text Encoding" in order to have both the more colloquial "Text" and the technical "Encoding" there and to align with Apple's terminology. Safari's corresponding submenu is labeled "Text Encoding". 5. Fix default text encoding: More specific than option 3 and 4, but longer I think "default" suggests technically a somewhat wrong thing considering how there default is understood to be a now-obsolete browser setting and not "whatever was initially determined for this page this time". However, if we're OK with a longer string, others suggested "Force Automatic Text Encoding" and observed that text editors have a "Reopen with Encoding" feature. In the browser context and with the automation here, the closest to the latter would be "Reload with Automatic Text Encoding". It's unclear if the nuance contemplated here reaches the users of this feature, since if we assume that users in the regions where the feature is used the most use localized versions of Firefox, users will see the Japanese, Traditional Chinese, or Simplified Chinese translation (and, to lesser extent, other non-Latin-script translations). Flags: needinfo?(hsivonen) - needinfo?(mwalkington) Henri Sivonen (:hsivonen) Assignee [003] Comment 21 * 9 months ago I expect users who use this feature to be aware of the conceptual source of the problem before invoking the UI even if they couldn't identify the exact pair of wrong and right encoding in a particular case. That is, for users of the feature, I expect the exact verb not to be the critical factor and I expect them to be looking for the "Text Encoding" part. Meridel [:meridel] Comment 22 [3d7] * 9 months ago Thanks, Henri. I appreciate you asking the DOM team and sharing context. Using "automatic" is a bit confusing because I assume what the user sees before trying to fix the issue is 'automatically' there. New and remaining options: 1. Force text encoding (leave it at this?) 2. Manual text encoding (implies that whatever showed up by default or automatically is not correct and a one-off, manual fix is needed) 3. Repair text encoding (this is pretty pedantic but "repair" might not promise as much as "fix" since "repair" is the act of repairing something -- the outcome is not promised -- while "fix" is the action itself) 4. Unscramble text encoding (in articles on this topic I am seeing people use the word 'unscramble' to describe fixing text encoding) 5. Override text encoding Flags: needinfo?(mwalkington) - needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee Comment 23 [003] * 9 months ago * Edited (In reply to Meridel [:meridel] from comment #22) 1. Force text encoding (leave it at this?) I think this is less informative than Override. This one also doesn't explain why and this seems less suggestive of changing it than Override. 2. Manual text encoding (implies that whatever showed up by default or automatically is not correct and a one-off, manual fix is needed) I think this is somewhat weird in the sense that what this patch is doing is taking away the possibility of manually choosing which one and leaving only the option to trigger the automated guessing. 3. Repair text encoding (this is pretty pedantic but "repair" might not promise as much as "fix" since "repair" is the act of repairing something -- the outcome is not promised -- while "fix" is the action itself) I agree that in English this seems better than Fix. The nuance between Repair and Fix might be lost in some translations. 4. Unscramble text encoding (in articles on this topic I am seeing people use the word 'unscramble' to describe fixing text encoding) I think this could work, although what's being unscrambled is, pedantically, the text and not the text encoding. 5. Override text encoding How about I change the menu item and the button label to "Repair Text Encoding", keep the button tooltip as "Guess text encoding from page content", and land? Flags: needinfo?(hsivonen) - needinfo?(mwalkington) Meridel [:meridel] Comment 24 [3d7] * 9 months ago Yes, I am happy with that solution, with a couple minor tweaks: Label: Repair text encoding (can we make this sentence case? Or are you treating this as a proper feature name? For Proton/MR1, we are moving to sentence case in all core UI) Tooltlip: Guess correct text encoding from page content (added "correct" to be more specific and based on your comment above that 'What the user wants to do is to override the text encoding that they visually believe to be wrong with the correct one. ' If you are good with these tweaks, we should then get Flod review. Flags: needinfo?(mwalkington) - needinfo?(hsivonen) Phabricator Automation Updated [a08] * 9 months ago Attachment #9217363 - Attachment description: Bug 1687635 part 1 - Replace Text Encoding submenu with Override Text Encoding item. - Bug 1687635 part 1 - Replace Text Encoding submenu with Repair Text Encoding item. Phabricator Automation Updated [a08] * 9 months ago Attachment #9217373 - Attachment description: Bug 1687635 part 2 - Disable Override Text Encoding when known not to have effect. - Bug 1687635 part 2 - Disable Repair Text Encoding when known not to have effect. Henri Sivonen (:hsivonen) Assignee [003] Comment 25 * 9 months ago (In reply to Meridel [:meridel] from comment #24) Yes, I am happy with that solution, with a couple minor tweaks: Label: Repair text encoding (can we make this sentence case? Or are you treating this as a proper feature name? For Proton/MR1, we are moving to sentence case in all core UI) Tooltlip: Guess correct text encoding from page content (added "correct" to be more specific and based on your comment above that 'What the user wants to do is to override the text encoding that they visually believe to be wrong with the correct one. ' Thanks! If you are good with these tweaks, we should then get Flod review. flod, I'm needinfoing you in case Phabricator doesn't surface the review request in this case. (Meridel clarified off-bug that the menu item should use the case "Repair Text Encoding" while the button label should use the case "Repair text encoding".) Flags: needinfo?(hsivonen) - needinfo?(francesco.lodolo) Henri Sivonen (:hsivonen) Assignee [003] Updated * 9 months ago Attachment #9217363 - Flags: ui-review?(mwalkington) Francesco Lodolo [:flod] Comment 26 [1c4] * 9 months ago (In reply to Henri Sivonen (:hsivonen) from comment #25) flod, I'm needinfoing you in case Phabricator doesn't surface the review request in this case. (Meridel clarified off-bug that the menu item should use the case "Repair Text Encoding" while the button label should use the case "Repair text encoding".) Done. Note that blocking reviewer is a safer choice for a patch that has already been reviewed ;-) Flags: needinfo?(francesco.lodolo) Pulsebot Comment 27 [4c5] * 9 months ago Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9f57ec83cdc6 part 1 - Replace Text Encoding submenu with Repair Text Encoding item. r=Gijs,fluent-reviewers,flod https://hg.mozilla.org/integration/autoland/rev/b0b7678a6781 part 2 - Disable Repair Text Encoding when known not to have effect. r=emk Iulian Moraru Comment 28 [348] * 9 months ago * Edited Backed out for causing bc failures on browser_967000_button_charEncoding.js. Push with failures Failure log Backout link This also failed on test verify: https://treeherder.mozilla.org/jobs? repo=autoland&revision=b0b7678a67810f52147536f5ca65070ec476b36d& searchStr=tv&selectedTaskRun=DFdR8g3GR96vvDjglEyu1Q.0 Flags: needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee [003] Comment 29 * 9 months ago (In reply to Francesco Lodolo [:flod] from comment #26) (In reply to Henri Sivonen (:hsivonen) from comment #25) flod, I'm needinfoing you in case Phabricator doesn't surface the review request in this case. (Meridel clarified off-bug that the menu item should use the case "Repair Text Encoding" while the button label should use the case "Repair text encoding".) Done. Note that blocking reviewer is a safer choice for a patch that has already been reviewed ;-) Thanks. (In reply to Iulian Moraru from comment #28) Backed out for causing bc failures on browser_967000_button_charEncoding.js. Push with failures Failure log Backout link This also failed on test verify: https://treeherder.mozilla.org/ jobs?repo=autoland&revision= b0b7678a67810f52147536f5ca65070ec476b36d&searchStr=tv& selectedTaskRun=DFdR8g3GR96vvDjglEyu1Q.0 Well, this is strange. The test passes locally individually and didn't show up on try: https://treeherder.mozilla.org/jobs?repo=try&revision= 5568003c9b92038e1f98f3bab81bf4d0383c341f&selectedTaskRun= BYNrt36STWajNuwQQ8SN3w.0 Flags: needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee [003] Comment 30 * 9 months ago Ah, it's a TSAN failure. Probably the test needs to wait. It already has an earlier need to wait. Henri Sivonen (:hsivonen) Assignee [003] Comment 31 * 9 months ago https://treeherder.mozilla.org/#/jobs?repo=try&revision= 633d34e0fc8d9200b6fbb7172edb51d9aa99eaab Pulsebot Comment 32 [4c5] * 9 months ago Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/701981112733 part 1 - Replace Text Encoding submenu with Repair Text Encoding item. r=Gijs,fluent-reviewers,flod https://hg.mozilla.org/integration/autoland/rev/bd95df8be7ca part 2 - Disable Repair Text Encoding when known not to have effect. r=emk Dorel Luca [:dluca] Comment 33 [059] * 9 months ago Backed out 2 changesets (bug 1687635) for Browser-chrome failures in browser/components/customizableui/test/ browser_967000_button_charEncoding.js. CLOSED TREE Log: https://treeherder.mozilla.org/logviewer?job_id=340926820&repo= autoland&lineNumber=2975 https://treeherder.mozilla.org/logviewer?job_id=340926842&repo= autoland&lineNumber=2642 Backout: https://hg.mozilla.org/integration/autoland/rev/ 08357f132f6f8d08eb3e5601c82e2b4dae6ea8ec Flags: needinfo?(hsivonen) Henri Sivonen (:hsivonen) Assignee [003] Comment 34 * 9 months ago The failures are in a part of the test that (before the attempt to work around this failure) were not changed by the patch on the test side and were at least not supposed to be changed by the patch on the browser UI side. Also, the failures are clearly intermittent, because they didn't happen on the landing itself but on a later run. Gijs, do you have ideas of what goes wrong and what the action should be? Flags: needinfo?(hsivonen) - needinfo?(gijskruitbosch+bugs) :Gijs (away until Feb 21; he/him) Comment 35 [664] * 9 months ago * Edited (In reply to Henri Sivonen (:hsivonen) from comment #34) The failures are in a part of the test that (before the attempt to work around this failure) were not changed by the patch on the test side and were at least not supposed to be changed by the patch on the browser UI side. Also, the failures are clearly intermittent, because they didn't happen on the landing itself but on a later run. Well, the TV jobs were orange on the initial push. :-) Gijs, do you have ideas of what goes wrong and what the action should be? The test indicates that the button should be disabled initially but isn't, so the question is... what should be disabling the button, and is that not happening, or is something else untowards happening that then re-enables it, before we check? Adding logging to your patch and pushing to try (if you can't reproduce locally with --verify) should help elucidate. I left some more notes on phabricator. Flags: needinfo?(gijskruitbosch+bugs) Henri Sivonen (:hsivonen) Assignee [003] Comment 36 * 9 months ago Try run with logging: https://treeherder.mozilla.org/#/jobs?repo=try&revision= bd6bf67de73c118932aa3f898d5d255d7f9f4f0b Henri Sivonen (:hsivonen) Assignee [003] Comment 37 * 9 months ago Without the waiting: https://treeherder.mozilla.org/#/jobs?repo=try&revision= 2355ab40b87be0f2e679a4d516652676e2124964 Henri Sivonen (:hsivonen) Assignee [003] Comment 38 * 9 months ago I think the cause is this async section: https://searchfox.org/mozilla-central/rev/ 2b372b94ce057097a6ef8eb725f209faa9d1dc4d/browser/components/ customizableui/CustomizeMode.jsm#555 And I think the test has was already bogus on this point but this changed something about the timing in an unlucky way. Henri Sivonen (:hsivonen) Assignee [003] Comment 39 * 9 months ago Let's see if this fixes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision= 0ca18418a8189bf955c4f7a57e6a203ee4091eb7 Henri Sivonen (:hsivonen) Assignee [003] Comment 40 * 9 months ago (In reply to Henri Sivonen (:hsivonen) from comment #39) Let's see if this fixes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision= 0ca18418a8189bf955c4f7a57e6a203ee4091eb7 It doesn't. I'm very tempted to remove the assertion from the test, since I can see that things are OK in manual testing. Henri Sivonen (:hsivonen) Assignee [003] Comment 41 * 9 months ago (In reply to Henri Sivonen (:hsivonen) from comment #40) (In reply to Henri Sivonen (:hsivonen) from comment #39) Let's see if this fixes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision= 0ca18418a8189bf955c4f7a57e6a203ee4091eb7 It doesn't. One more logging round: https://treeherder.mozilla.org/#/jobs?repo=try&revision= 06461d4b66935179846c5e382982401411148c6d I'm very tempted to remove the assertion from the test, since I can see that things are OK in manual testing. It doesn't seem useful to put a lot of time into chasing this. If there's a real user-facing bug, it would only show up on a rare proportion of times when the user closes the customization view, which is a rare event itself. Henri Sivonen (:hsivonen) Assignee [003] Comment 42 * 9 months ago Trying with the assertion removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision= 18702a42703984720d1d6c687e1e0358b1a8f7f3 Henri Sivonen (:hsivonen) Assignee [003] Comment 43 * 9 months ago One more: https://treeherder.mozilla.org/#/jobs?repo=try&revision= 5a089f6be2f62ac06dd10521fd4050194ee93f95 Henri Sivonen (:hsivonen) Assignee [003] Comment 44 * 9 months ago https://treeherder.mozilla.org/#/jobs?repo=try&revision= 2c7c56847c48a4b180e9a738e2a041f88bea66da Henri Sivonen (:hsivonen) Assignee [003] Comment 45 * 9 months ago (In reply to Henri Sivonen (:hsivonen) from comment #44) https://treeherder.mozilla.org/#/jobs?repo=try&revision= 2c7c56847c48a4b180e9a738e2a041f88bea66da The run that passes looks suspicious on terms of expected test logging being missing. Henri Sivonen (:hsivonen) Assignee [003] Comment 46 * 9 months ago (In reply to Henri Sivonen (:hsivonen) from comment #41) It doesn't seem useful to put a lot of time into chasing this. If there's a real user-facing bug, it would only show up on a rare proportion of times when the user closes the customization view, which is a rare event itself. I uploaded a version of the patch that removes the assertion to Phabricator. Gijs, is it OK to land this without the problematic assertion (once the soft freeze is over)? As noted above, chasing this particular assertion looks like a really bad use of developer time in terms of the possible user impact: Manual testing suggests this is not a problem. And even if it was, it would be pretty harmless in itself (button enabled when it shouldn't and not doing anything harmful if pressed), would take place only upon exiting customization (rare event), and would be resolved upon the next page load anyway. Flags: needinfo?(gijskruitbosch+bugs) :Gijs (away until Feb 21; he/him) Comment 47 [664] * 9 months ago (In reply to Henri Sivonen (:hsivonen) from comment #45) (In reply to Henri Sivonen (:hsivonen) from comment #44) https://treeherder.mozilla.org/#/jobs?repo=try&revision= 2c7c56847c48a4b180e9a738e2a041f88bea66da The run that passes looks suspicious on terms of expected test logging being missing. Logging from tests gets buffered on infra, and elided from the complete log output if the test passes, unless you call SimpleTest.requestCompleteLog. This is because otherwise log files for test runs would be even more huge than they are at the moment. This is also why the log output includes stuff like "Buffered messages logged at