Closed Bug 1077211 Opened 10 years ago Closed 4 years ago

Grammar problem in message emitted HTML parser

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
firefox81 --- fixed

People

(Reporter: sideshowbarker, Assigned: sideshowbarker)

References

(
URL
)

Details

Attachments

(3 files, 2 obsolete files)

patch.grammar-fix
970 bytes, patch
hsivonen
: review+
Details | Diff | Splinter Review
Translation for Gecko
2.21 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
Bug 1077211 - Fix grammar problem in HTML parser error message.
47 bytes, text/x-phabricator-request
Details | Review
See view-source:http://jsbin.com/giquho/2/ Hover over the red `<body>` start tag in the "view source" view of the document at the URL above, and you'll see this message: 'An “body” start tag seen but an element of the same type was already open.' Note that it reads '*An* “body” start tag' instead of the expected '*A* “body” start tag'. This issue was originally reported at https://github.com/validator/validator/issues/1 and
Attached patch proposed patch (obsolete) — — Splinter Review
This proposed patch fixes the grammar problem by changing the error message for this case to read, e.g., 'Start tag “body” seen but an element of the same type was already open.' That actually brings the error message for this case to more consistency with the format of error messages emitted from other parts of the existing code; e.g., "End tag \u201C" + name + "\u201D seen, but there were open elements." http://hg.mozilla.org/projects/htmlparser/file/ddc1fa48fcc9/src/nu/validator/htmlparser/impl/TreeBuilder.java#l6110
Summary: Grammar error in message emitted by error-reporting facet of the HTML parser (exposed in "View source") → Grammar problem in message emitted by error-reporting facet of the HTML parser (exposed in "View source")
Attachment #8499282 - Flags: review?(hsivonen)
Comment on attachment 8499282 [details] [diff] [review] proposed patch The new formulation is OK, but the patch fails to remove the old line, hence r-. (Also, the patch isn't enough to fix Gecko. For that, you need to revise the string for errFooSeenWhenFooOpen accordingly and mint a new key for it, e.g. errFooSeenWhenFooOpen2.)
Attachment #8499282 - Flags: review?(hsivonen) → review-
Attachment #8499282 - Attachment is obsolete: true
Attachment #8519725 - Flags: review?(hsivonen)
(In reply to Henri Sivonen (:hsivonen) from comment #2) > Comment on attachment 8499282 [details] [diff] [review] > proposed patch > > The new formulation is OK, but the patch fails to remove the old line, hence > r-. Oops, OK, please see the updated patch > (Also, the patch isn't enough to fix Gecko. For that, you need to revise > the string for errFooSeenWhenFooOpen accordingly You mean somewhere else in addition to http://hg.mozilla.org/projects/htmlparser/file/default/src/nu/validator/htmlparser/impl/TreeBuilder.java ? > and mint a new key for it, e.g. errFooSeenWhenFooOpen2.) OK. Where do I do that? In the following files? https://hg.mozilla.org/mozilla-central/file/default/dom/locales/en-US/chrome/layout/htmlparser.properties https://hg.mozilla.org/mozilla-central/file/default/parser/html/nsHtml5TreeBuilderCppSupplement.h Also, I don't completely know enough about how the parser code operates within gecko to understand why it's necessary to mint a new key. Why can't I just change the existing errFooSeenWhenFooOpen message/key and have the sufficient?
Comment on attachment 8519725 [details] [diff] [review] patch.grammar-fix (In reply to Michael[tm] Smith from comment #4) > (In reply to Henri Sivonen (:hsivonen) from comment #2) > > Comment on attachment 8499282 [details] [diff] [review] > > proposed patch > > > > The new formulation is OK, but the patch fails to remove the old line, hence > > r-. > > Oops, OK, please see the updated patch r+ as a Java-only thing, but it would be nice to keep Gecko in sync. > > (Also, the patch isn't enough to fix Gecko. For that, you need to revise > > the string for errFooSeenWhenFooOpen accordingly > > You mean somewhere else in addition to > http://hg.mozilla.org/projects/htmlparser/file/default/src/nu/validator/ > htmlparser/impl/TreeBuilder.java ? Yes, see below: > > and mint a new key for it, e.g. errFooSeenWhenFooOpen2.) > > OK. Where do I do that? In the following files? > > https://hg.mozilla.org/mozilla-central/file/default/dom/locales/en-US/chrome/ > layout/htmlparser.properties > https://hg.mozilla.org/mozilla-central/file/default/parser/html/ > nsHtml5TreeBuilderCppSupplement.h Yes. > Also, I don't completely know enough about how the parser code operates > within gecko to understand why it's necessary to mint a new key. Why can't I > just change the existing errFooSeenWhenFooOpen message/key and have the > sufficient? The new key is needed to trigger retranslation in localizations. Basically any time a string changes, the key needs to change. (Even for types, because non-U.S. English locales may have imported the typos.)
Attachment #8519725 - Flags: review?(hsivonen) → review+
Summary: Grammar problem in message emitted by error-reporting facet of the HTML parser (exposed in "View source") → Grammar problem in message emitted HTML parser
Attachment #8591559 - Flags: review?(hsivonen)
Attachment #8591559 - Flags: review?(hsivonen) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

Comment on attachment 9160905 [details]
Bug 1077211 - Fix grammar problem in HTML parser error message. DONTBUILD NPOTB

Revision D81996 was moved to bug 1003273. Setting attachment 9160905 [details] to obsolete.

Attachment #9160905 - Attachment is obsolete: true
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0456f06f7b4f Fix grammar problem in HTML parser error message. r=flod,alchen

https://hg.mozilla.org/mozilla-central/rev/0456f06f7b4f

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox81: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Assignee: hsivonen → mike
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: