font-size text attribute should be exposed in pt units

RESOLVED FIXED in mozilla1.9.1b4

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: surkov, Assigned: davidb)

Tracking

(Blocks: 1 bug, {access, verified1.9.1})

unspecified
mozilla1.9.1b4
access, verified1.9.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

9 years ago
font-size text attribute should be exposed in pt units, not in px units.

Comment 1

9 years ago
Since "pt units" are only meaningful for printing, not for electronic display screens, font size should be translated from "px units" to "pt units" when the text is displayed by File > Print Preview and by File > Print.  Just my opinion.

Comment 2

9 years ago
(In reply to comment #1)
> Since "pt units" are only meaningful for printing, not for electronic display
> screens, font size should be translated from "px units" to "pt units" when the
> text is displayed by File > Print Preview and by File > Print.  Just my
> opinion.

Upon further consideration:  what you are really asking is for Firefox to behave as a What-You-See-Is-What-You-Get (WYSIWYG "wizzi-wig") document editor, like Microsoft Word or Open Office Writer, at least when Firefox displays a web page or another .HTML file which is to be printed as though it were a document.  

From what I can recall about "editors" after the introduction of Windows 3.1 and 3.2, developing WYSIWYG software is not a trivial task (the matter of software patents aside).  When you really need to see what the web page or other .HTML content that is displayed by Firefox will look like when it is printed, and control the text output by specifying the point size for each font, then maybe you should use File > Save Page as PDF, then use a "word processor" that can read the .PDF format to edit the file before you print it.

Comment 3

9 years ago
Aaron, can you give a little more background as to why, when designing the IAccessible2 and AT-SPI APIs, it was decided that these point sizes should be in pt, not px? Comments 1 and 2 raise a few valid points here, IMO.
(Reporter)

Comment 4

9 years ago
Ocie, I'm not sure we talk about the same thing. We expose 'font-size' as text
attribute in accessibilty API (for IAccessible2 and ATK). IAccessible2 team
requested to expose 'font-size' text attribute in 'pt' units. I quite not sure
why they want to have it in 'pt' units. Concerning your previous post ('pt'
units don't make sense for displays) I'll ask them about this. Possibly all
they want is AT (screen readers) should have unique unit for 'font-size'
attribute and it doesn't matter what is the unit exactly (I keep in mind the
case when AT need to compare font sizes (bigger/smaller) and don't need to know
what this means exactly).

Comment 5

9 years ago
Alexander, the following is from Bug 465237 which I posted about the effects of using the Firefox "Minimum font size" feature (accessed with Tools menu > Options > Content - Fonts & Colors - Advanced > Fonts dialog):

--- Comment #6 from Mardeg <mardeg@nerdshack.com>  2008-11-30 01:31:37 PST ---
>From the Accessibility Extension report on avg.com: http://tinyurl.com/avgpage
Violoation: Set font sizes using relative measurements (not px, pt, in, cm,
mm). 
Occurences: 190
Class: Text Sizing

This is a valid Tech Evangelism bug, AVG needs to fix the code on it's page to
make it more accessible to things like a larger minimum font size.

>From your report "some pages on any given website may not be adversely affected
while others are" you will likely find the ones that are make similar coding
mistakes, and only the developers of those sites can fix that, sorry.

Here's the accessibility extension:
https://addons.mozilla.org/en-US/firefox/addon/5809
The report was generated using "beta rule set"
-------------------------------------------------------------------------------

As the "violation" message says, the web page designer/coder should use "relative measurements" and not px (pixel), pt (point size), in (inches), cm (centimeters), mm (millimeters).

Frankly, it has been many years since I worked as an analyst/programmer and I don't know anything about current "accessibility" standards or their adoption.  I don't know whether HTML 2.0 has a provision for relative sizes of letters, digits and symbols (text characters).  I realize that "relative measurements" would be an advantage in re-sizing a web page, or the display of any document, but I have no idea how "relative measurements" are created and implemented in practice.  Perhaps the IAccessible2 team is looking for a "unit" of relative size, such as a fraction or a multiple:

(a) 1000 is the largest size for a font, each smaller size is expressed as a fraction of it; for example, size 500 would be one half the size of the largest size.

(b) 100 is the smallest size for a font, each larger size is expressed as a multiple of it; for example, size 150 would be one-and-one-half larger than the smallest size, and 200 would be twice the size of the smallest size.

Note that using "1000" instead of "1.00" for the largest size, and using "100"  instead of "1.00" for the smallest size, avoids calculations with decimal fractions, so that all of the arithmetic is done with integers. :-)


That said, here is an exercise that shows me (and perhaps you, too) that with Firefox, I do not get, printed on the paper page, what I see on the display screen: 

(1) I used File > Print Preview to display the web page on which this message is displayed.  Then I measured the length of the following line, with a ruler:

> Since "pt units" are only meaningful for printing, not for electronic display

(2) On my display monitor, Print Preview displays that line as about 8.5 inches long (the width of the most commonly used paper page for printing letters and documents -- 8.5 in. x 11 in.).  The characters in that displayed text are larger than the size of the same characters, as they are displayed by Firefox for this Bugzilla page.

(3) On this Bugzilla display, the line is only about 7 inches long, and the characters are smaller in size than shown by Print Preview.  

(4) When I print the page with the HP Deskjet D1420, the same line is 6.38 inches long, but the size of the characters appears to be a little bit larger than they are displayed by Bugzilla.  

So, what I see displayed by Firefox on my computer's monitor is not the same as what I will get when the document is printed.  In contrast, a WSYIWYG document editor will display text characters the same size on the monitor as they will be when they are printed.  But as I said before, making WYSIWYG is definitely not a trivial task.
(Reporter)

Comment 6

9 years ago
Pete, could you comment?

Comment 7

9 years ago
The idea was that for all kinds of apps the screen readers would only have to deal with one kind of unit for font size and that pt was closer to how end users think of font sizes.  Aaron also commented in a separate email:

I think pt is right. First, if the AT wants px it can look at the character bounds using IAccessibleText Second, pt is usually how end users think about font sizes. It's how one picks a font size when editing in basically any application.

Comment 8

9 years ago
Pete & Alexander,

Do you know which "unit" that Firefox 3.0.4 uses when we exercise the options on the Tools > Options > Content: Fonts & Colors (Default drop-downs) and Advanced > Fonts dialog??  

The numbers seem "relative" to me, because when I choose "16" as the "Minimum font size", the size of the characters that are displayed for Firefox menu Help > Help Contents is quite _considerably_ larger than the size of the characters that are displayed for Bugzilla messages.  But that difference in size also exists  when the "Minimum font size" is set to "None"; the Help Contents characters are much larger than the relatively tiny characters displayed for Bugzilla messages.  So, apparently the Firefox configured size is relative to the default font size chosen for the page by its designer (the option "Allow pages to choose their own fonts, ..." is enabled).

With regard to the end-users, is a specific "unit" necessary?  If you choose 
point size, then some end-users will reasonably expect the characters to appear just as they would on a printed document with that point size.  In effect, a "screen reader" then should be What-You-See-Is-What-You-Get (WYSIWYG), which is ordinarily the case now when "editing in basically any application" (that runs under Windows, anyway).  How feasible that would be for a "screen reader", I don't know.
(Assignee)

Comment 9

9 years ago
Okay, I think we need to provide what the external APIs (IA2) expect for font-size units, so I think we must do a conversion to pt here. I'll give it a shot.

Note ATK_TEXT_ATTR_SIZE doesn't specify units AFAIK
Assignee: nobody → david.bolter
Status: NEW → ASSIGNED

Comment 10

9 years ago
David, we don't use those outdated ATK text attribute constants. We expose exactly the same text attributes for ATK as we do in IA2. This came after discussion with Bill Haneman orignally, and later Will Walker agreed that we should use what the Linux Foundation IA2 group decided on.
(Assignee)

Comment 11

9 years ago
Perfect. Thanks Aaron. I keep finding old ATK docs, and of course I never used the text attributes much (or at all) in GOK.
(Reporter)

Comment 12

9 years ago
David, just FYI, sometime ago I started the patch but didn't understand completely layout code needed for this. I'm not sure I will get back this to really soon, so if you feel hot on this in the meantime then do it.
(Assignee)

Comment 13

9 years ago
The more I dig into this, the more I agree that "pt" might be a miss-match for font-size in hypertext. I'm tempted to try and fix this in spec. Also, adding checks for unit type in nsHyperTextAccessible::GetDefaultTextAttributes might be a perf pooper.
(Assignee)

Comment 14

9 years ago
Created attachment 357391 [details] [diff] [review]
possible fix (no tests yet)
(Assignee)

Comment 15

9 years ago
Comment on attachment 357391 [details] [diff] [review]
possible fix (no tests yet)

Marco, in manual tests this looks like it does the right conversion for various types css font-size values. I'm not sure of the right automated tests because the conversion to pt might be dependent on the device context. Maybe it would be enough just to check the last two characters of our font-size text attribute for "pt"?
Attachment #357391 - Flags: review?(marco.zehe)
(Reporter)

Comment 16

9 years ago
Few things 1) it's preferable to keep the code inside of textattr file, 2) create specific class like we have for background-color 3) share code with layout (that's why I said I didn't understand completely needed layout code for this. If we want this to fix this bug very quick then please file following up bugs and add XXX comments.
Error output from Mochitests looks like this with the patch:
1056 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area1 - got "12pt", expected "16px"
1070 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area2 - got "12pt", expected "16px"
1091 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area3 - got "12pt", expected "16px"
1111 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area4 - got "12pt", expected "16px"
1127 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area5 - got "12pt", expected "16px"
1144 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area6 - got "12pt", expected "16px"
1173 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area7 - got "12pt", expected "16px"
1211 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area8 - got "10pt", expected "13.3333px"

As you can see, you're converting!

I also did some manual tests using both a build with your patch and one regular nightly, and the point sizes you return usually match what I see with the JAWS cursor, which is the mouse pointer emulator, and which always grabs the point size displayed on-screen.

But I agree with Alexander, you shouldn't dump this algorithm right into nsHyperTextAccessible. I'd suggest to go with the approach he suggests in comment #16.
(Assignee)

Comment 18

9 years ago
Surkov, Marco, this is exactly the feedback I was looking for, thanks.

(In reply to comment #16)
> Few things 1) it's preferable to keep the code inside of textattr file, 2)
> create specific class like we have for background-color 3) share code with
> layout (that's why I said I didn't understand completely needed layout code for
> this. If we want this to fix this bug very quick then please file following up
> bugs and add XXX comments.

1) Sounds good to me.
2) OK
3) I'm not sure what 'share' means here. Do you think I should create a new layout method that does the conversion?

(In reply to comment #17)
Thanks Marco, I will have to update the tests of course :)
(In reply to comment #18)
> 3) I'm not sure what 'share' means here. Do you think I should create a new
> layout method that does the conversion?

Yes, I believe that's what Alex meant. I know he did this for some of the other text attribute things. The big patch that first introduced everything was in bug 345759.
(Assignee)

Comment 20

9 years ago
Created attachment 357708 [details] [diff] [review]
moved smarts into nsTextUtils, modified test_textattrs

I thought about making a static util method for px to pt conversion but in the end, I followed Surkov (and Marco's) advice and created a class akin to the background color attr stuff.
Attachment #357391 - Attachment is obsolete: true
Attachment #357708 - Flags: review?(marco.zehe)
Attachment #357391 - Flags: review?(marco.zehe)
With this, I get one failure on Windows:
1218 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area8 - got "10pt", expected "8pt"
(Assignee)

Comment 22

9 years ago
Created attachment 357803 [details] [diff] [review]
Same as before but with OS sniff.

It appears font-size for inputs on Linux are 10pt, and on Mac/Windows, 8pt.
Attachment #357708 - Attachment is obsolete: true
Attachment #357803 - Flags: review?(marco.zehe)
Attachment #357708 - Flags: review?(marco.zehe)
(Reporter)

Comment 23

9 years ago
Created attachment 357807 [details] [diff] [review]
my unended patch

possibly it may be interesting
(Reporter)

Updated

9 years ago
Attachment #357807 - Attachment is patch: true
Attachment #357807 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 24

9 years ago
I think nsFontTextAttr::Equal can just do whatever is most performant. I'll tweak it.
(Assignee)

Comment 25

9 years ago
Created attachment 357817 [details] [diff] [review]
combined patch

WIP, this passes the tests. Need to figure out why I needed to divide by magic 4.
Attachment #357803 - Attachment is obsolete: true
Attachment #357807 - Attachment is obsolete: true
Attachment #357803 - Flags: review?(marco.zehe)
Nit:
>+            "font-size": (MAC)? "8pt" : "10pt",

Please insert a space before the ? to comply with generally used style. Thanks!
(Assignee)

Comment 27

8 years ago
Created attachment 357838 [details] [diff] [review]
cleaner

I went back to using the recommended device context. Apparently TWIPs are deprecated with the full page zoom work. I have made the patch simpler though, and ::Equal comparison doesn't convert to pt anymore (better perf). We can do the conversion if ATs request it.

All tests pass for me. Marco can you test in Windows. Will be nice to close this.
Attachment #357817 - Attachment is obsolete: true
Attachment #357838 - Flags: review?(marco.zehe)

Comment 28

8 years ago
Comment on attachment 357838 [details] [diff] [review]
cleaner

For the platform-specific stuff, I guess you should just use #ifdef XP_WIN etc.

Updated

8 years ago
Attachment #357838 - Flags: review?(marco.zehe) → review+
Comment on attachment 357838 [details] [diff] [review]
cleaner

This one works! Good job!

Updated

8 years ago
Attachment #357838 - Flags: review?(surkov.alexander)
Comment on attachment 357838 [details] [diff] [review]
cleaner

Requesting additional review from Alex.
(Assignee)

Comment 31

8 years ago
(In reply to comment #28)
> (From update of attachment 357838 [details] [diff] [review])
> For the platform-specific stuff, I guess you should just use #ifdef XP_WIN etc.

Hi Aaron, we only really need to check the os in the tests... so I did it the JS way.
(Reporter)

Comment 32

8 years ago
(In reply to comment #31)
> (In reply to comment #28)
> > (From update of attachment 357838 [details] [diff] [review] [details])
> > For the platform-specific stuff, I guess you should just use #ifdef XP_WIN etc.
> 
> Hi Aaron, we only really need to check the os in the tests... so I did it the
> JS way.

I guess just Aaron meant it's preferable to use preprocessor directives, just I'm not sure if they can be used in mochitests (though iirc they can be used in any file included into makefile.in but I may be wrong).
(Reporter)

Comment 33

8 years ago
Comment on attachment 357838 [details] [diff] [review]
cleaner

font-size isn't exposed as text attribute any more, only as a part of default text attributes.
Attachment #357838 - Flags: review?(surkov.alexander) → review-
(Assignee)

Comment 34

8 years ago
(In reply to comment #33)
> (From update of attachment 357838 [details] [diff] [review])
> font-size isn't exposed as text attribute any more, only as a part of default
> text attributes.

It never was exposed as non-default on the ATK side AFAICT. I'll add the logic in for when the defaults flag is false for completeness assuming this makes a difference on the Windows side.
(Assignee)

Comment 35

8 years ago
Created attachment 357974 [details] [diff] [review]
more complete, includes non-default exposure.
Attachment #357838 - Attachment is obsolete: true
Attachment #357974 - Flags: review?(surkov.alexander)
(Assignee)

Updated

8 years ago
Attachment #357974 - Flags: review?(marco.zehe)

Updated

8 years ago
Attachment #357974 - Attachment is patch: true
Attachment #357974 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 36

8 years ago
nit: / XXX todo: consider moving to layout module - it's better "consider to share this code with layout module" or similar otherwise it sounds for me layout module doesn't have similar code. And I would like to see a XXX bug xxxxx :)
(Reporter)

Comment 37

8 years ago
nit: NS_ENSURE_TRUE(dc, NS_ERROR_UNEXPECTED); it's NS_ENSURE_STATE(dc)
(Assignee)

Comment 38

8 years ago
(In reply to comment #36)
Created bug 474621
Will update patch.
(Assignee)

Comment 39

8 years ago
Created attachment 357985 [details] [diff] [review]
addresses surkov's nits
Attachment #357974 - Attachment is obsolete: true
Attachment #357985 - Flags: review?(marco.zehe)
Attachment #357974 - Flags: review?(surkov.alexander)
Attachment #357974 - Flags: review?(marco.zehe)

Updated

8 years ago
Attachment #357985 - Flags: review?(marco.zehe) → review+
Comment on attachment 357985 [details] [diff] [review]
addresses surkov's nits

All tests pass on Windows as well, thanks!
(Assignee)

Updated

8 years ago
Attachment #357985 - Flags: review?(surkov.alexander)
(Assignee)

Comment 41

8 years ago
Comment on attachment 357985 [details] [diff] [review]
addresses surkov's nits

Surkov, want another look?
(Reporter)

Comment 42

8 years ago
Comment on attachment 357985 [details] [diff] [review]
addresses surkov's nits


> static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
>-  // CSS name            CSS value        Attribute name              Attribute name
>+  // CSS name            CSS value        Attribute name              Attribute value

nit: what have been changed here?
>+PRBool
>+nsFontSizeTextAttr::Get(nsAString& aValue)
>+{
>+  // Do not expose "background-color" text attribute if its value is matched
>+  // with the default value.
>+  nscoord size = GetFontSize(mFrame);

nit: fontSize variable name is better?

>+  if (mRootFrame && size == GetFontSize(mRootFrame))
>+    return PR_FALSE;
>+
>+  nsAutoString value;
>+  // XXX todo: consider sharing this code with layout module (bug 474621)
>+  nsIDeviceContext *dc = mFrame->PresContext()->DeviceContext();
>+  NS_ENSURE_STATE(dc);

don't use NS_ENSURE_STATE here because your method is boolean.

>+  float inches = (double)(GetFontSize(mFrame)) / (double)dc->AppUnitsPerInch();

nit: iirc, c style cast are deprecated, it's preferable to use c++ style casts - static_cast

>+  int pts = inches * 72 + .5;

I would like to get sr for this code from Roc.

>+/**
>+ * Class is used for the work with "font-size" text attribute. It is
>+ * used in nsHyperTextAccessible.
>+ */
>+class nsFontSizeTextAttr : public nsTextAttr
>+{
>+public:
>+  nsFontSizeTextAttr(nsIFrame *aFrame, nsIFrame *aRootFrame);
>+  
>+  // nsTextAttr
>+  virtual PRBool Equal(nsIDOMElement *aElm);
>+

nit: would be nice to put here the comment // nsFontSizeTextAttr

>+  /**
>+   * @note  IA2 requires pt size as integer.

nit: technically it's not correct because this comment doesn't address needs of ATK. Any @note follows method description

>+   * 
>+   * returns font size in pts as integer.

nit: since return value is not font size then it's preferable to put right comment here. Here I would like to see comment when we return PR_TRUE and when PR_FALSE.

>+   */
>+  virtual PRBool Get(nsAString& aValue);
>+
>+private:
>+  /**
>+   * Return font size for the given frame.
>+   *
>+   *

nit: excess "*"

>+   * @param aFrame      [in] the given frame to query font-size
>+   * @return            font size
>+   */
>+   nscoord GetFontSize(nsIFrame *aFrame);
>+
>+  nsIFrame *mFrame;
>+  nsIFrame *mRootFrame;
>+  nsCOMPtr<nsIDOMCSSStyleDeclaration> mStyleDecl;

unused member

>     nsAutoString value, oldValue;
>-    if (textAttr.Get(name, value))
>+    if (textAttr.Get(name, value)) {
>       attributes->SetStringProperty(name, value, oldValue);
>+    }

nit: this change isn't necessary, my own preference do not use {} for single if statement.

>   }
>+  
>+  nsIFrame *sourceFrame = nsCoreUtils::GetFrameFor(element);
>+  NS_ENSURE_TRUE(sourceFrame, NS_ERROR_UNEXPECTED);

nit: use NS_ENSURE_STATE

>+  // font size must be exposed in pt units

nit: this comment is excess, nsHyperTextAccessible is wrong place to care about units I guess.

>+  nsAutoString value;
>+  nsFontSizeTextAttr fontSizeTextAttr(sourceFrame, nsnull);
>+  fontSizeTextAttr.Get(value);
>+  nsAccUtils::SetAccAttr(attributes,
>+                         nsAccessibilityAtoms::fontSize, value);
>+  
>+  value.Truncate(0);

nit: iirc Truncate() defines default attribute value, it's no need to pass 0.

nit: new line please

>+  // set font background color
>+  nsBackgroundTextAttr backgroundTextAttr(sourceFrame, nsnull);
>+  if (backgroundTextAttr.Get(value)) {

I think you can drop 'if' statement here like you have for font-size case because Get() method can't return false for default text attributes case. Additionally I think it makes sense to add comment to Get() method description,  

>+////////////////////////////////////////////////////////////////////////////////
>+// OS detect
>+const MAC = (navigator.platform.indexOf("Mac") != -1)? true : false;
>+const LINUX = (navigator.platform.indexOf("Linux") != -1)? true : false;
>+const WIN = (navigator.platform.indexOf("Win") != -1)? true : false;

>           gComputedStyle = document.defaultView.getComputedStyle(node, "");
>+          var fontSize;
>+          if (navigator.platform.indexOf)

what is this?


I like to look at new patch.
Attachment #357985 - Flags: review?(surkov.alexander)
(Assignee)

Comment 43

8 years ago
(In reply to comment #42)
> (From update of attachment 357985 [details] [diff] [review])
> 
> > static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
> >-  // CSS name            CSS value        Attribute name              Attribute name
> >+  // CSS name            CSS value        Attribute name              Attribute value
> 
> nit: what have been changed here?

I changed 'Attribute name' to 'Attribute value' -- is that correct?
(Assignee)

Comment 44

8 years ago
(In reply to comment #42)
> (From update of attachment 357985 [details] [diff] [review])
> >+  nsAutoString value;
> >+  // XXX todo: consider sharing this code with layout module (bug 474621)
> >+  nsIDeviceContext *dc = mFrame->PresContext()->DeviceContext();
> >+  NS_ENSURE_STATE(dc);
> 
> don't use NS_ENSURE_STATE here because your method is boolean.

I'm confused, please see your comment 37 ;)
(Reporter)

Comment 45

8 years ago
(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 357985 [details] [diff] [review] [details])
> > 
> > > static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
> > >-  // CSS name            CSS value        Attribute name              Attribute name
> > >+  // CSS name            CSS value        Attribute name              Attribute value
> > 
> > nit: what have been changed here?
> 
> I changed 'Attribute name' to 'Attribute value' -- is that correct?

you're right, sorry
(Reporter)

Comment 46

8 years ago
(In reply to comment #44)
> (In reply to comment #42)
> > (From update of attachment 357985 [details] [diff] [review] [details])
> > >+  nsAutoString value;
> > >+  // XXX todo: consider sharing this code with layout module (bug 474621)
> > >+  nsIDeviceContext *dc = mFrame->PresContext()->DeviceContext();
> > >+  NS_ENSURE_STATE(dc);
> > 
> > don't use NS_ENSURE_STATE here because your method is boolean.
> 
> I'm confused, please see your comment 37 ;)

Yea, I know, but there I said about replacement NS_ENSURE_TRUE on NS_ENSURE_SUCCESS - that was correct. But I didn't noticed return value is boolean. It means you can't use these macros there because these macros returns nsresult.
(Assignee)

Comment 47

8 years ago
Sorry about these bits, I rushed my patch up too soon without self-review. I'm
glad you caught them!

> >+  nsCOMPtr<nsIDOMCSSStyleDeclaration> mStyleDecl;
> 
> unused member

Nice catch.

> 
> >     nsAutoString value, oldValue;
> >-    if (textAttr.Get(name, value))
> >+    if (textAttr.Get(name, value)) {
> >       attributes->SetStringProperty(name, value, oldValue);
> >+    }
> 
> nit: this change isn't necessary, my own preference do not use {} for single if
> statement.

OK. That was a left-over because I had multiple statements.

> I think you can drop 'if' statement here like you have for font-size case
> because Get() method can't return false for default text attributes case.
> Additionally I think it makes sense to add comment to Get() method description, 


> >+          if (navigator.platform.indexOf)
> 
> what is this?

Beats me! Removing.
(Assignee)

Comment 48

8 years ago
Created attachment 358182 [details]
Removed cruft, tweaked comments, improved cast etc.
Attachment #358182 - Flags: review?(surkov.alexander)
(Assignee)

Comment 49

8 years ago
Created attachment 358183 [details] [diff] [review]
(repost - with proper mime type)
Attachment #357985 - Attachment is obsolete: true
Attachment #358182 - Attachment is obsolete: true
Attachment #358183 - Flags: review?(surkov.alexander)
Attachment #358182 - Flags: review?(surkov.alexander)
(Reporter)

Comment 50

8 years ago
Comment on attachment 358183 [details] [diff] [review]
(repost - with proper mime type)


>+PRBool
>+nsFontSizeTextAttr::Get(nsAString& aValue)
>+{
>+  // Do not expose "background-color" text attribute if its value is matched
>+  // with the default value.

nit: oops, "background-color"? ;)

>+  nscoord fontsize = GetFontSize(mFrame);

nit: I like fontSize, but it's up to you.

>+  if (mRootFrame && fontsize == GetFontSize(mRootFrame))
>+    return PR_FALSE;
>+
>+  // Convert from nscoord to pt.
>+  //
>+  // Note: according to IA2, "The conversion doesn't have to be exact.
>+  // The intent is to give the user a feel for the size of the text."

nit: would be nice to have two phrases it doesn't go contrary to ATK.

> class nsBackgroundTextAttr : public nsTextAttr
> {
> public:
>   nsBackgroundTextAttr(nsIFrame *aFrame, nsIFrame *aRootFrame);
>   
>-  // nsTextAttr
>+  // nsBackgroundTextAttr
>   virtual PRBool Equal(nsIDOMElement *aElm);

nit: I meant put //nTextAttr before Equal() and put //nsBackgroundColor before Get().

>+  // nsFontSizeTextAttr
>+  virtual PRBool Equal(nsIDOMElement *aElm);

nit: the same

>+  
>+  nsIFrame *sourceFrame = nsCoreUtils::GetFrameFor(element);
>+  NS_ENSURE_TRUE(sourceFrame, NS_ERROR_UNEXPECTED);

nit: NS_ENSURE_STATE

>+    value.Truncate(0);

nit: empty argument please

>           gComputedStyle = document.defaultView.getComputedStyle(node, "");
>+          var fontSize;

nit: fontSize isn't used, right?

rest looks great, thank you for patience, r=me
Attachment #358183 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 51

8 years ago
Created attachment 358188 [details] [diff] [review]
patch to go in

Thanks to my reviewers, this is another davidb learning stuff patch :)

Marco, can you do the push?
(Assignee)

Comment 52

8 years ago
(In reply to comment #50)
> (From update of attachment 358183 [details] [diff] [review])

> >+  // nsBackgroundTextAttr
> >   virtual PRBool Equal(nsIDOMElement *aElm);
> 
> nit: I meant put //nTextAttr before Equal() and put //nsBackgroundColor before
> Get().
> 

Just to follow up... Surkov, now this nit makes sense to me, since we are using a virtual method here!  Nice to get clarity as I didn't understand the prior related nit.
Pushed on Davidb's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/c1c28d2c16f4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Comment on attachment 358188 [details] [diff] [review]
patch to go in

This is needed for completion of text attribute support in accessibility, which is a new Firefox 3.1 feature.
Attachment #358188 - Flags: approval1.9.1?
I'd note that using layout's ratio to convert app units to points isn't necessarily going to be the same definition of points that other windows apps use.  Our internal DPI can be different due to the layout.css.dpi pref.  If you wanted to get a more reliable conversion to a number meaningful to other windows apps, I think you should convert to device pixels using Gecko APIs, and then convert device pixels to points based on the number returned from a call to the Win32 API GetDeviceCaps(dc, LOGPIXELSY).
(In reply to comment #55)
> I'd note that using layout's ratio to convert app units to points isn't
> necessarily going to be the same definition of points that other windows apps
> use.  Our internal DPI can be different due to the layout.css.dpi pref.

The ones who requested that we expose PT instead of PX were indeed the people on the IAccessible2 standards group, and IA2 is Win32. However, the conversion applies to all platforms. Do you know if something similar to what you suggest below applies to Linux and Mac OS as well?
> If you wanted to get a more reliable conversion to a number meaningful to
> other windows apps, I think you should convert to device pixels using Gecko
> APIs, and then convert device pixels to points based on the number returned
> from a call to the Win32 API GetDeviceCaps(dc, LOGPIXELSY).

OK, if I got this right, the only thing we'd need to do then is to make these platform-specific calls in our platform a11y wrappers.
Again, it would be good to know if Linux and Mac also have these peculiarities.

Thanks for the suggestion! I believe we can put this adjustment into a separate bug. My manual testing showed that this mostly works accurately currently, so I don't think there's a need to back this out and retry. Rather, we can develop on top of the current solution.
(Assignee)

Comment 57

8 years ago
(In reply to comment #55)
Great info thanks! I'll wait to see answers to Marco's x-platform question.
(Assignee)

Comment 58

8 years ago
Actually I realize you are probably too busy to go hunting down px to pt conversions on other platforms. Note there is a comment on point conversions in the IA2 spec:
"The conversion doesn't have to be exact. The intent is to give the user a feel for the size of the text."

Updated

8 years ago
Flags: in-testsuite+
Comment on attachment 358188 [details] [diff] [review]
patch to go in

a191=beltzner
Attachment #358188 - Flags: approval1.9.1? → approval1.9.1+
Pushed to mozilla-1.9.1 on David's behalf in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/17822d168308
Keywords: access, fixed1.9.1
Target Milestone: --- → mozilla1.9.1b4
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.