Fixes #6206: Fix enlargement of text in RTE #6447

bansalnitish · 2019-03-13T13:54:18Z

ankita240796

LGTM from code owner's perspective, Thanks @bansalnitish!

codecov-io · 2019-03-13T14:36:37Z

Codecov Report

Merging #6447 into develop will decrease coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #6447      +/-   ##
===========================================
- Coverage    75.76%   75.73%   -0.04%     
===========================================
  Files          992      969      -23     
  Lines        78607    78350     -257     
  Branches      4649     4625      -24     
===========================================
- Hits         59560    59335     -225     
+ Misses       19047    19015      -32

Impacted Files	Coverage Δ
...loration_editor/editor_tab/ExplorationEditorTab.js	`33.33% <0%> (-17.42%)`	⬇️
schema_utils.py	`95.9% <0%> (-4.1%)`	⬇️
...s/dev/head/domain/utilities/LanguageUtilService.js	`84.21% <0%> (-3.03%)`	⬇️
core/templates/dev/head/pages/library/Library.js	`23.72% <0%> (-2.51%)`	⬇️
.../directives/ItemSelectionInputValidationService.js	`67.16% <0%> (-1.67%)`	⬇️
core/controllers/profile.py	`88.48% <0%> (-1.4%)`	⬇️
core/domain/visualization_registry.py	`93.33% <0%> (-0.61%)`	⬇️
...ev/head/expressions/ExpressionSyntaxTreeService.js	`98.28% <0%> (-0.43%)`	⬇️
core/controllers/base.py	`95.43% <0%> (-0.39%)`	⬇️
.../dev/head/domain/question/QuestionObjectFactory.js	`93.44% <0%> (-0.21%)`	⬇️
... and 164 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11c73a7...eb69d3d. Read the comment docs.

YashJipkate

Hi @bansalnitish, I have one concern. PTAL. Thanks!

YashJipkate · 2019-03-13T16:30:15Z

core/templates/dev/head/css/oppia.css

+  nested inside the p and span tag. !important is added to override
+  the inline css.*/
+.oppia-rte p span {
+  font-size: 1em !important;


Is this the only way to get this done? I am saying this because we already have an issue #5097 regarding removal of !important from the codebase.

I agree with @YashJipkate. Please find a way to avoid !important, or explain the line of reasoning (with evidence) that causes you to conclude that doing so is not possible. Typically you can avoid the use of !important by adding more specificity in the CSS rule.

See e.g. https://uxengineer.com/css-specificity-avoid-important-css/#incremental-specificty

I don't think we can remove !important from here. As discussed on the issue thread, a span tag gets introduced before the text (which the issue of CKEditor) with a inline style. !important is a only way to remove that inline style.

Wait -- I don't understand something. If the inline style is coming from a CSS class in oppia.css, why is that style being added inline to the span tag? Is there some JS code that is doing that? Normally styles in oppia.css classes will show up as being part of the class in the CSS inspector, it wouldn't be "unpacked" and added to the DOM element directly.

The span tag is introduced by CKEditor. It comes as style=font-size:1.6 em. It's taking the same font size whatever is mentioned in the body tag in oppia.css.

Ah ok. Can you add all this information to the comment as justification of why this cannot be resolved without using !important? Once you've done that, I think this can go in.

Thanks!

Sure, thanks!

seanlip · 2019-03-15T06:00:16Z

core/templates/dev/head/css/oppia.css

+   This size is taken similar to font-size in the body element. There is as
+   such no way to remove !important as of now, its a CKEditor bug
+   (refer issue #6206). */
+.oppia-rte p span {


Why do we do this for both oppia-rte and oppia-rte-editor?

I did this because this bug appeared to happen during the editing mode as well after saving. So I need to fix it at both of these places. Thanks!

seanlip · 2019-03-15T06:00:38Z

core/templates/dev/head/css/oppia.css

@@ -358,6 +358,21 @@ p:last-child {
  margin: 1.4em 0;
 }

+/* This will prevent the enlargment of the text in the RTE. A span tag gets


spelling: enlargement

seanlip · 2019-03-15T06:00:44Z

core/templates/dev/head/css/oppia.css

@@ -358,6 +358,21 @@ p:last-child {
  margin: 1.4em 0;
 }

+/* This will prevent the enlargment of the text in the RTE. A span tag gets
+   introduced when a user selects the text from the end of first line, goes to the
+   second one and then write some text to replace that. The span tag carries


write --> writes

seanlip · 2019-03-15T06:01:21Z

core/templates/dev/head/css/oppia.css

+   along with it a style=font-size:1.6em;, due to which the text is enlarged.
+   This size is taken similar to font-size in the body element. There is as
+   such no way to remove !important as of now, its a CKEditor bug
+   (refer issue #6206). */


issue #6206 on Oppia

(otherwise sounds like it's #6206 on CKEditor repo)

seanlip · 2019-03-15T06:01:59Z

core/templates/dev/head/css/oppia.css

+   introduced when a user selects the text from the end of first line, goes to the
+   second one and then write some text to replace that. The span tag carries
+   along with it a style=font-size:1.6em;, due to which the text is enlarged.
+   This size is taken similar to font-size in the body element. There is as


This size seems to be taken from the font-size from the body element's CSS class, but we do not know what JS code is causing it to be inlined in the span tag.

seanlip

Thanks @bansalnitish!

seanlip · 2019-03-20T22:13:47Z

core/templates/dev/head/css/oppia.css

+   along with it a style=font-size:1.6em;, due to which the text is enlarged.
+   This size seems to be taken from the font-size from the body element's
+   CSS class, but we do not know what JS code is causing it to be inlined in the
+   span tag. There is as such no way to remove !important as of now, its a CKEditor


nit: it's not its.

("It's" is short for "it is" and is appropriate here.)

seanlip · 2019-03-20T22:14:02Z

@YashJipkate, I think this needs your review. Thanks!

YashJipkate

LGTM! Thanks @bansalnitish!

seanlip · 2019-03-22T19:25:15Z

Hm. Any idea why it says "The Travis CI build is still in progress"? The Details page suggests that they've completed.

/cc @oppia/dev-workflow-team

apb7 · 2019-03-23T11:48:35Z

@bansalnitish, I think your branch is not up-to-date to develop. Can you please update your branch? Lint checks now run on CircleCi. Thanks!

Fix enlargement of text inRTE

6e3e652

bansalnitish requested a review from ankita240796 as a code owner March 13, 2019 13:54

bansalnitish requested a review from seanlip March 13, 2019 13:54

ankita240796 approved these changes Mar 13, 2019

View reviewed changes

YashJipkate suggested changes Mar 13, 2019

View reviewed changes

bansalnitish added the CHANGELOG: Bug fixes label Mar 13, 2019

bansalnitish added 2 commits March 14, 2019 20:03

Added a detailed comment

878ffda

Merge branch 'develop' into fixLargetext

bfc684e

seanlip requested changes Mar 15, 2019

View reviewed changes

Updated comment

a775006

vojtechjelinek assigned YashJipkate and seanlip Mar 20, 2019

seanlip approved these changes Mar 20, 2019

View reviewed changes

seanlip reviewed Mar 20, 2019

View reviewed changes

seanlip removed their assignment Mar 20, 2019

YashJipkate approved these changes Mar 22, 2019

View reviewed changes

YashJipkate removed their assignment Mar 22, 2019

bansalnitish added 2 commits March 27, 2019 22:50

Updated comment

87bfc20

Merge branch 'develop' into fixLargetext

eb69d3d

bansalnitish merged commit 6cf5a9d into oppia:develop Mar 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #6206: Fix enlargement of text in RTE #6447

Fixes #6206: Fix enlargement of text in RTE #6447

bansalnitish commented Mar 13, 2019

ankita240796 left a comment

codecov-io commented Mar 13, 2019 •

edited

Loading

YashJipkate left a comment

YashJipkate Mar 13, 2019

seanlip Mar 13, 2019 •

edited

Loading

seanlip Mar 13, 2019

bansalnitish Mar 13, 2019

seanlip Mar 13, 2019

bansalnitish Mar 13, 2019

seanlip Mar 14, 2019

bansalnitish Mar 14, 2019

seanlip Mar 15, 2019

bansalnitish Mar 20, 2019

seanlip Mar 15, 2019

bansalnitish Mar 20, 2019

seanlip Mar 15, 2019

bansalnitish Mar 20, 2019

seanlip Mar 15, 2019

bansalnitish Mar 20, 2019

seanlip Mar 15, 2019

bansalnitish Mar 20, 2019

seanlip left a comment •

edited

Loading

seanlip Mar 20, 2019

seanlip commented Mar 20, 2019

YashJipkate left a comment

seanlip commented Mar 22, 2019

apb7 commented Mar 23, 2019

Fixes #6206: Fix enlargement of text in RTE #6447

Fixes #6206: Fix enlargement of text in RTE #6447

Conversation

bansalnitish commented Mar 13, 2019

ankita240796 left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 13, 2019 • edited Loading

Codecov Report

YashJipkate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Mar 20, 2019

YashJipkate left a comment

Choose a reason for hiding this comment

seanlip commented Mar 22, 2019

apb7 commented Mar 23, 2019

codecov-io commented Mar 13, 2019 •

edited

Loading

seanlip Mar 13, 2019 •

edited

Loading

seanlip left a comment •

edited

Loading